fix: list-only kinds (OLM packagemanifests) poll instead of dying in the watch loop (#141) - #146
Conversation
…the watch loop (#141) PackageManifest is served by OLM's packageserver, which supports only get/list: the LIST->WATCH loop cleared and re-seeded the store five times (table flicker), then killed the view with an error toast. - discovery keeps list-only kinds instead of dropping them, marked ResourceMeta.watchable=False (verbs are finally captured). - watch_objects degrades those kinds - and any server that advertises watch but rejects it with 405 - to periodic re-LIST diffing (LIST_POLL_INTERVAL=30s): upserts for present rows, DELETED for vanished ones, so the stream stays alive and incremental and the view keeps rendering. Other watch errors still propagate to the manager retry loop. - WatchManager treats 405 like 403 as deterministic (no retry burn), but keeps the LISTed rows - only authorization denials purge. Fixes #141 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds polling support for Kubernetes resources that support LIST but not WATCH, notably OLM PackageManifests.
Changes:
- Records resource watch capability during discovery.
- Adds periodic LIST diffing and intended 405 fallback.
- Makes WatchManager treat 405 responses as deterministic.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/korvid/k8s/discovery.py |
Adds ResourceMeta.watchable. |
src/korvid/k8s/client.py |
Implements polling and watch fallback. |
src/korvid/core/watch.py |
Stops retrying deterministic 405 errors. |
tests/k8s/test_discovery.py |
Tests list-only discovery. |
tests/k8s/test_client.py |
Tests polling and fallback behavior. |
tests/core/test_watch.py |
Tests manager handling of 405 errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![my-reviewer-agent[bot]](https://avatars.githubusercontent.com/in/4343504?s=60&v=4){kind=small}
There was a problem hiding this comment.
총평 (issue #141 — list-only kinds 폴링 폴백): 깔끔한 구현입니다. discovery가 watch verb 없는 kind를 watchable=False로 보존하고, watch_objects가 LIST+주기적 재-LIST diff 스트림으로 우아하게 강등되며(스토어 clear 없음, 뷰는 active 유지), 런타임 405도 결정적 오류로 처리해 재시도 낭비를 막습니다. _watch_loop에서 405가 reconnect purge 이전에 반환되므로 'LIST된 행 유지' 주장도 head 코드로 확인했습니다. _list_path 헬퍼 추출로 중복 제거된 것도 좋고, 5개 테스트(RED-first)가 discovery/폴링 diff/405 폴백/비-405 전파/manager 단일 보고를 모두 커버합니다.
Suggestion (경계 사례): watch 스트림이 이벤트를 얼마간 소비한 뒤 중간에 405로 전환되는 경우, watch 이벤트로 ADDED된 행은 known에 반영되지 않습니다. 이후 폴링 라운드에서 그 행이 사라지면 known에도 current에도 없어 DELETED가 영원히 발생하지 않아 스토어에 잔류할 수 있습니다. 실제로 405는 스트림 시작 시점에 결정적으로 발생하므로 현실적 위험은 낮지만, 405 폴백 시 known 대신 스토어 스냅샷을 시드하거나 watch 이벤트를 known에도 upsert하면 닫을 수 있는 갭입니다.
APPROVE
Review round 1 on #146: the raw-watch callable surfaces HTTP errors as ApiStatusError via _raise_for_status - not ApiException - so a real packageserver 405 bypassed the fallback and killed the view through the manager anyway. The watch phase now catches both exception types (both carry .status/.reason); non-405 statuses re-raise as ApiStatusError as before. RED tests: test_watch_405_falls_back_to_list_polling now models the adapter's ApiStatusError; ApiException(405) and the non-405 ApiStatusError passthrough are pinned separately. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/korvid/k8s/client.py:490
- For a non-405 raw-watch failure,
excis already anApiStatusErrorwhosebodywas populated by_raise_for_status(client.py:1491-1504). Reconstructing it here drops that response body, so this does not actually propagate the error unchanged as the new test documents. Re-raise an existingApiStatusErrorand only translate the Kubernetes client'sApiException.
status = int(getattr(exc, "status", 0) or 0)
if status != 405:
raise ApiStatusError(status, str(getattr(exc, "reason", "") or "")) from exc
There was a problem hiding this comment.
재리뷰 (신규 커밋 1개, 라운드 1 지적 반영 확인) — APPROVE.
raw watch 어댑터가 _raise_for_status를 통해 ApiStatusError를 던지므로 실제 packageserver 405가 기존 ApiException-only 핸들러를 우회해 뷰를 죽이던 문제를 정확히 수정했습니다. 405 폴백이 이제 두 예외 타입을 모두 잡고, non-405는 종전과 같이 ApiStatusError로 전파됩니다. RED 테스트가 어댑터의 실제 예외 타입(ApiStatusError(405))을 모델링하도록 교체되었고, ApiException(405) 폴백과 non-405 ApiStatusError passthrough가 각각 별도 테스트로 고정된 점 좋습니다.
인라인에 사소한 Suggestion 1건(재래핑 시 .body 유실) 남겼습니다.
Round 2 advisory on #146 (both reviewers): re-wrapping a non-405 ApiStatusError dropped .body, which same-status disambiguation (PDB denial vs APF throttling, issue #109) depends on. The re-raise now carries body through (ApiException exposes one too); pinned by the extended non-405 passthrough test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
재리뷰 (신규 커밋 1건, e69a9f8 이후): 지난 라운드에서 지적한 non-405 재래핑 시 .body 유실 문제가 정확히 수정되었습니다.
watch_objects의 non-405 경로가 이제ApiStatusError(status, reason, body)로 body까지 보존하며, ApiException 쪽도getattr(exc, "body", ...)로 안전하게 커버합니다. 동일 status 구분(PDB 거부 vs APF 스로틀링, #109)이 의존하는 정보가 유지됩니다.- 테스트가
excinfo.value.body == '{"kind":"Status"}'를 명시적으로 단언하도록 강화되어 회귀를 고정합니다.
추가 지적사항 없습니다. APPROVE
on_resources_updated can be dispatched during app teardown, after the screen stack is emptied: App.screen then raises ScreenStackError, which run_test() re-raises and fails whichever test happened to be running - the intermittent test_ctx_switch CI failures (also seen on PRs #140 and #146; the CI traceback names this exact frame). No screen simply means no tree to refresh. Fixes #147 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes #141
Problem
:view packagemanifestson an OLM cluster was unusable:PackageManifestis served by OLM's aggregated packageserver, which supports onlyget/list— nowatch. korvid's LIST→WATCH loop cleared and re-seeded the store on every retry (table flicker for ~5s), then killed the view with an error toast and removed it fromWatchManager.activeforever.Fix
Follow the metrics-poller precedent, generically:
_parse_resource_listkeeps list-only kinds instead of dropping them (they were previously invisible on servers that declare verbs honestly), markingResourceMeta.watchable=False.watch_objectsdegrades to LIST + poll. For unwatchable kinds — and for servers that advertise watch but reject it with 405 at runtime — the stream falls back to periodic re-LIST diffing (LIST_POLL_INTERVAL = 30s): upserts for present rows,DELETEDfor vanished ones. The stream stays alive and incremental — no store clear, no flicker, view stays inactiveso rendering, hierarchy lookups, and the operator install flow work unchanged. Other watch errors still propagate to the manager's retry/report loop.WatchManagertreats 405 as deterministic (like 403 — no retry burn), but unlike a 403 the rows the successful LIST delivered stay visible; only authorization denials purge the bucket.Tests (TDD — RED first)
test_discover_resources_keeps_list_only_kinds_as_unwatchable— list-only kinds discovered withwatchable=False; kinds without evenliststay excluded.test_unwatchable_kind_polls_lists_and_diffs_instead_of_watching— poll rounds upsert/delete-diff; the Watch API is never touched.test_watch_405_falls_back_to_list_polling— watch-advertising-but-405 servers degrade to polling.test_watch_non_405_api_exception_still_raises— other errors keep the manager in charge.test_405_reports_once_without_retries_and_keeps_listed_rows— manager: one attempt, one report, LISTed rows survive.Full gate (
make check) green: 2825 passed.