Reschedule on project and same-ID task content changes

[thusser]

Summary

• Scheduler._update_schedule() diffed task IDs only, so a project priority change or a same-ID task content change never triggered a reschedule even once the archive re-polled.
• Fixes an assignment-order bug where self._projects was overwritten before it could ever be diffed against the previous download.
• Adds project content-diff (model_dump(), keyed by code, mirroring PortalTaskArchive._update()) and a new Scheduler._changed_task_ids() helper for same-ID task content diffs.
• New asymmetry vs. removed/added: a content change on the currently-running task always forces a reschedule (its new priority can reorder everything after it), unlike a removed running task, which is ending anyway. A content change on a task that isn't in the current schedule is skipped, reusing the existing schedule-membership guard.
• Follow-up commit (post-review): adds Project.updated_at for round-trip compatibility with pyobs-portal#134 (without it, Project's extra="forbid" model rejects the portal's payload on every poll — see review below); excludes updated_at from the content-diff comparisons so a no-op re-save doesn't force a spurious reschedule; broadens the private-attr regression test; adds coverage for project removal and a mixed removed+changed poll.

Closes #848 (pyobs-core half — the portal-side `/api/last_task_update/` marker fix is pyobs-portal#134). Not independently landable as originally scoped — see the PR comment below; that's now fixed in this PR.

Also filed #855: `PolymorphicBaseModel`'s custom serializer silently ignores `model_dump(exclude=/include=)` for every subclass (`Task`, `Script`, `Constraint`, `Merit`, `Target`), found while fixing this — worked around locally here, real fix is a separate, wider-scoped change.

Plan: `specs/plans/2026-09-01-scheduler-reschedule-on-project-and-task-changes.md`

Test plan

• Unit tests in `tests/modules/robotic/test_scheduler.py`: project content-diff (priority/users/public, parametrized, plus removal and an `updated_at`-only no-op), same-ID task content-diff (in schedule / not in schedule / on the running task / `updated_at`-only no-op / mixed with a removed running task), `_changed_task_ids()` unit tests including a private-attr-mutation regression guard.
• Regression test in `tests/robotic/storage/portal/test_portal_archives.py` for `Project.updated_at` round-tripping through `_get_projects()`.
• Existing scheduler test suite unaffected.
• Full non-integration suite: 1786 passed, 0 failed.
• `ruff` / `pyrefly` clean; `black` pre-commit hook passed.

[thusser]

Review (verified against the PR head a374458d)

Independently re-ran the claimed checks in the PR worktree:

Check	Result
pytest tests/modules/robotic/test_scheduler.py	60 passed (50 baseline + 10 new)
Full non-integration suite	1762 passed, 28 failed — all failures pre-existing (the same files fail on develop: cli/test_pyobsd.py, mixins/test_fitsheader.py, camera/test_basevideo.py, telescope/test_dummysolartelescope.py; environment/flaky, unrelated to this PR)
ruff check + black --check on changed files	clean

The scheduler logic itself is sound: the assignment-order fix, the model_dump()-keyed-by-code project diff mirroring PortalTaskArchive._update(), _changed_task_ids() (private attrs excluded from the dump by construction), the running-task asymmetry, and the extended downgrade guards all check out. The new tests assert the interesting transitions, including get_schedule() not being awaited on the running-task path.

1. BLOCKER — core Project will reject the portal's new updated_at; the two PRs are not independently landable

pyobs-portal #134 adds Project.updated_at, and ProjectSerializer uses fields="__all__", so /api/projects/ will start emitting updated_at. Core's Project (pyobs/robotic/task.py:152) extends pyobs.utils.serialization.BaseModel (extra="forbid") and has no such field, so PortalTaskArchive._get_projects() (pyobs/robotic/storage/portal/taskarchive.py:117) will raise ValidationError: Extra inputs are not permitted on every poll once the portal half lands. Verified directly:

Project.model_validate({..., "updated_at": "2026-09-01T10:20:30Z"})
→ ValidationError [type=extra_forbidden]      (Task.model_validate with the field: OK)

Consequences: _update() dies before _get_tasks(), _last_marker never advances, and the poll loop retries forever with one throttled ERROR per minute — task and project updates silently stop flowing to the scheduler. Since this PR's whole purpose is to react to those updates, the feature is inert end-to-end until the round-trip field lands.

The precedent is already in this repo: e550423e ("fix: accept updated_at field from robotic-backend (#84)") added updated_at: str | None = None to Task together with a regression test (test_task_get_tasks_from_portal_accepts_updated_at, tests/robotic/storage/portal/test_portal_archives.py:154). The same one-liner + test is needed for Project — best placed in this PR (or an immediate companion):

class Project(BaseModel):
    code: str
    name: str = ""
    priority: float | None = Field(ge=0.0, le=9999.0, default=1.0)
    users: list[str] = Field(default_factory=list)
    public: bool = False
    updated_at: str | None = None  # round-trip: portal emits it (pyobs-portal#134); extra="forbid"

Both plans' "independently landable" claims should be amended accordingly.

2. LOW — model_dump() also compares Task.updated_at; the docstring overstates "only real content differences"

updated_at is a declared field, so it's part of the dump — consistent with PortalTaskArchive._update() and the marker design (any save moves the marker, the archive fires, the scheduler sees the timestamp differ), but it means a no-op save (e.g. a DRF PATCH with unchanged data still calls Task.save()) counts as "changed" and forces a full reschedule when the task is scheduled. If only semantic changes should trigger, exclude updated_at from the comparison; otherwise the _changed_task_ids docstring should say "any field difference, including updated_at". The same applies to projects_changed once finding 1's field lands.

3. LOW — the private-attr regression test is narrower than the plan asked for

The plan wanted a task run through the real self._scheduler.schedule(...) (or resolve_target()/can_run()) and diffed against a fresh copy; the implementation pins only _cant_run_reason. The mechanism test covers the core risk, but a single end-to-end scheduling round-trip would also guard _resolved_target/_running_script and any future public-field mutation — cheap insurance given the livelock history.

Nits

• PR body says "51 pre-existing tests"; the file has 50 on develop (60 on this branch).
• No test for a mixed poll (removed + changed together, e.g. running task removed while an off-schedule task changed) or for project removal — both handled by the code but untested.

[thusser]

Addressed in 3b06cf1:

1. BLOCKER (Project.updated_at) — fixed. Added updated_at: str | None = None to core's Project, mirroring the Task.updated_at precedent (e550423e) exactly, plus test_task_get_projects_from_portal_accepts_updated_at mirroring that precedent's own regression test. Amended both plans' "independently landable" claims.
2. LOW (updated_at in content comparisons) — fixed, but not via model_dump(exclude=...): Task is a PolymorphicBaseModel, and its custom @model_serializer (pyobs/utils/serialization.py:44-49) builds its own dict and never calls handler(self), so it silently ignores exclude/include entirely. Filed as PolymorphicBaseModel's custom model_serializer ignores exclude/include (and other model_dump kwargs) #855 (separate, wider-scoped fix — it affects every PolymorphicBaseModel subclass, not just this comparison). Worked around locally with Scheduler._content_dump(), which dumps normally and pops the key afterward. Project isn't polymorphic so exclude= does work there as originally written.
3. LOW (private-attr regression narrower than planned) — broadened test_changed_task_ids_ignores_private_attr_mutation to also set _resolved_target/_running_script, not just _cant_run_reason. Didn't go all the way to a full self._scheduler.schedule(...) round-trip (harder to set up meaningfully with the polymorphic Script/Target machinery for marginal extra coverage) — flagging in case you want that pushed further.
4. Nits — added test_update_schedule_project_removed_triggers_update and a mixed-poll test (test_update_schedule_mixed_removed_running_and_changed_scheduled_triggers_update: running task removed + an unrelated, actually-scheduled task changed in the same poll — the latter must still force a reschedule). Didn't edit the PR body's stale "51" count since the description already needs a re-read anyway with these changes.

Full non-integration suite green again (1786 passed) after all of the above; ruff/black/pyrefly clean.

[thusser]

Follow-up review (head 3b06cf16) — all findings addressed, approve

Re-verified everything from the previous review against the new head in the PR worktree:

Previous finding	Status
BLOCKER — Project rejects the portal's updated_at (extra="forbid")	✅ Fixed: updated_at: str | None = None round-trip field on Project (mirroring Task's e550423e precedent) + test_task_get_projects_from_portal_accepts_updated_at archive regression test
LOW — model_dump() compares updated_at → no-op saves force reschedules	✅ Fixed, and it went deeper than the ask: updated_at is now excluded from both comparisons, with the PolymorphicBaseModel serializer-exclude trap found and worked around (_content_dump()), filed as #855
LOW — private-attr regression test narrower than the plan	✅ Fixed: now exercises _resolved_target/_running_script in addition to _cant_run_reason
Nits — missing project-removal and mixed removed+changed tests	✅ Fixed: test_update_schedule_project_removed_triggers_update + test_update_schedule_mixed_removed_running_and_changed_scheduled_triggers_update (the latter correctly exercises the len(changed)==0 guard on the removal downgrade)

Independent verification on the new head:

Check	Result
pytest tests/modules/robotic/test_scheduler.py tests/robotic/storage/portal/test_portal_archives.py	107 passed
ruff + black --check on the 4 changed files	clean
Serializer claim in _content_dump's docstring	confirmed empirically: Task.model_dump(exclude={"updated_at"}) still contains updated_at (the PolymorphicBaseModel serializer ignores exclude entirely), while Project.model_dump(exclude={"updated_at"}) correctly drops it — so the pop-workaround for Task and the exclude= for Project are each the right tool, and #855 is a real, correctly-scoped bug
Plan doc + PR description	updated with the coupling note and the addendum; both accurate

One small remaining observation (not blocking):

LOW — PortalTaskArchive._update() still compares with plain model_dump(), so updated_at still propagates one level. The archive (taskarchive.py:96-98) wasn't touched: a no-op re-save still moves the marker → archive re-polls → its content diff includes updated_at → fires on_tasks_changed → the scheduler re-downloads and re-diffs, then correctly concludes "no change" and skips the reschedule. End-to-end behavior is right, but a no-op save still triggers a full task/project re-download + scheduler _update_schedule() pass. Excluding updated_at from the archive's comparisons too (or reusing the same helper) would make no-op saves completely invisible and avoid the churn — optional, follow-up material.

Verdict: the blocker and all notes are resolved; this is ready to merge (together with pyobs-portal#134, per the corrected coupling note).

[thusser]

Verified all four points against the new head (3b06cf16) — agree with everything, and the follow-up review above confirms it independently:

• v0.8 #1/v0.8.3 #2: Project.updated_at round-trip + archive regression test pass; the PolymorphicBaseModel serializer claim is confirmed empirically (Task.model_dump(exclude={"updated_at"}) still contains updated_at, Project's doesn't) — the _content_dump() workaround is the right call, and PolymorphicBaseModel's custom model_serializer ignores exclude/include (and other model_dump kwargs) #855 is correctly scoped.
• v0.9 #3: agree the full schedule() round-trip adds marginal coverage for real setup cost; the broadened PrivateAttr test covers the mechanism that actually keeps the diff stable. Fine as-is.
• Entry points #4: the two added tests are exactly the missing cases; mixed removed-running + changed-scheduled correctly exercises the len(changed)==0 guard on the removal downgrade. The stale "51" count in the body is harmless with the description already rewritten.

Local re-run: test_scheduler.py + test_portal_archives.py → 107 passed; ruff/black clean on the four changed files. One non-blocking observation left (archive-level _update() still compares plain model_dump(), so no-op saves still trigger a re-download + scheduler re-diff before concluding "no change") — follow-up material, noted in the review above.

No further changes needed from my side — ready to merge together with pyobs-portal#134.

[thusser]

Rebased onto `develop` — it had advanced with PR #852 (issue #847) in the meantime, which conflicted at the design level, not just textually: #852 removed the "was one of the removed tasks actually in `get_schedule()`?" guard entirely, because `PortalObservationArchive`'s schedule cache is permanently empty by construction, so the guard always found nothing and silently discarded every real portal task removal.

My branch had reused that same guard for the new `changed_ids` set. Left in place, it would've reintroduced the identical bug one level up — every same-ID task content change silently swallowed on a portal deployment, defeating this issue's whole point. Dropped the guard for `changed_ids` too (never re-added it), matching #852's decision. Net effect: project changes, task add/remove, and same-ID content changes all now force a reschedule unconditionally, except the one remaining downgrade (a removal that's exactly the currently-running task ending on its own). This also simplifies the design — the running-task "asymmetry" and its bypass variable are gone, since there's no membership check left to bypass.

Rewrote the affected tests to match (no longer mocking/asserting on `get_schedule()` for content changes, mirroring #852's own `..._without_consulting_schedule_cache` naming). Full suite: 1800 passed (up from 1786 pre-merge, reflecting #852's own additions). Documented in the plan's new "Merge-conflict addendum" section.