From 43cbc2294fc7dd6b1e49c842674f78bd1a8d3a05 Mon Sep 17 00:00:00 2001 From: hellices Date: Mon, 3 Aug 2026 00:39:22 +0900 Subject: [PATCH 1/4] feat: pre-warm drill targets - no empty-view flash on push/pop (#157) Drilling deploy -> rs -> pods visibly happened in two steps: the pane switched to a just-cleared bucket (WatchManager.start clears before the LIST), rendered empty, and filled one network RTT later. Esc had the same shape on the way back (the parent watch was stopped on the way down, so navigating back re-cleared + re-LISTed). _drill_into and _pop_drill now warm the target first: start the watch while the current view is still up (a kind no pane displays renders nowhere), wait - bounded by DRILL_PREWARM_TIMEOUT - until the rows the transition will show exist (owned_by(parent_uid) for a push, any parent row for a pop), then run the unchanged push/pop+navigate transaction. _navigate_locked's start() is a no-op by then, the bucket is warm, and the single post-switch render lands with real rows. While waiting the status bar carries 'loading ' - the corvid busy indicator (#143) animates it. - a live watch (split pane) skips both restart and wait - timeout degrades to the old switch-then-fill, never worse - _stop_watch_if_unused reaps the pre-warmed stream when the drill lost its pane or raced a scope change - NavigationStack.peek() for the pop-side prewarm target test_concurrent_drill_and_navigate_stay_consistent now gates on the drill actually blocking inside the critical section instead of a sleep - the prewarm shifted the old timing assumption; the invariant it pins is unchanged. Closes #157 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/korvid/ui/app.py | 96 ++++++++++++++++++---- src/korvid/ui/navigation.py | 4 + tests/ui/test_drilldown.py | 160 +++++++++++++++++++++++++++++++++++- 3 files changed, 243 insertions(+), 17 deletions(-) diff --git a/src/korvid/ui/app.py b/src/korvid/ui/app.py index a9c9c90d..e3082283 100644 --- a/src/korvid/ui/app.py +++ b/src/korvid/ui/app.py @@ -2174,6 +2174,44 @@ async def _drill_down_selected(self, row_key: str) -> None: if error is not None: self.notify(error, severity="warning") + #: Longest a drill transition waits for the target view's initial LIST + #: before switching anyway (issue #157). A slow cluster degrades to the + #: old switch-then-fill behavior, never worse. + DRILL_PREWARM_TIMEOUT = 1.0 + + async def _prewarm_view( + self, + kind: str, + scope: str, + ready: Callable[[list[Summary]], bool], + ) -> None: + """Warm the drill target before the pane switches (issue #157). + + Starting the watch for a kind no pane displays renders nowhere, so + the LIST happens invisibly while the current view stays up; the + bounded wait ends as soon as `ready` sees the expected rows. The + subsequent `_navigate_locked` start() is then a no-op (the watch is + already running), the bucket is warm, and the single post-switch + render lands with real rows instead of flashing an empty table. + A live watch (another pane shows this kind/scope) is already warm - + restarting it would clear the bucket it serves. + """ + if (kind, scope) in self.watch_manager.active: + return + await self.watch_manager.start(kind, scope) + deadline = monotonic() + self.DRILL_PREWARM_TIMEOUT + with self._progress(f"loading {kind}"): + while monotonic() < deadline: + if ready(self.store.get(kind, scope)): + return + await asyncio.sleep(0.03) + + async def _stop_watch_if_unused(self, kind: str, scope: str) -> None: + """Reap a pre-warmed watch no pane ended up displaying (issue #157): + a drill that lost its pane (or its race) must not leak a stream.""" + if all((p.kind, p.scope) != (kind, scope) for p in self._panes): + await self.watch_manager.stop(kind, scope) + async def _drill_into(self, namespace: str, name: str) -> str | None: """Push a drill level for (namespace, name) in the current view and navigate to the child kind. Returns an error message, or None on success.""" @@ -2210,15 +2248,30 @@ async def _drill_into(self, namespace: str, name: str) -> str | None: # Capture before waiting on the lock: focus may move (or the pane may # close) while this drill queues behind another navigation. pane = self._pane - async with self._nav_lock: - if pane not in self._panes: - return None # the initiating pane was closed while queued - pane.drill.push(level) - try: - await self._navigate_locked(pane, child, None) - except BaseException: - pane.drill.pop() - raise + # Warm the child view first (issue #157): wait - bounded - until the + # rows this drill will show exist, so the switch renders once with + # content instead of flashing an empty table while the LIST runs. + prewarm_scope = pane.scope + await self._prewarm_view( + child, + prewarm_scope, + lambda rows: any(owned_by(r, uid) for r in rows), + ) + try: + async with self._nav_lock: + if pane not in self._panes: + return None # the initiating pane was closed while queued + pane.drill.push(level) + try: + await self._navigate_locked(pane, child, None) + except BaseException: + pane.drill.pop() + raise + finally: + # No-op when the navigation landed (the pane now displays the + # warmed kind/scope); reaps the stream when the drill lost its + # pane or raced a scope change. + await self._stop_watch_if_unused(child, prewarm_scope) self._render_table(pane.kind, only=pane) self._refresh_status() return None @@ -2230,13 +2283,24 @@ async def _pop_drill(self) -> bool: # Capture before waiting on the lock: focus may move (or the pane may # close) while this pop queues behind another navigation. pane = self._pane - async with self._nav_lock: - if pane not in self._panes: - return False # the initiating pane was closed while queued - popped = pane.drill.pop() - if popped is None: - return False - await self._navigate_locked(pane, popped.parent_kind, None) + peeked = pane.drill.peek() + if peeked is None: + return False + # Warm the parent view first (issue #157): its watch was stopped + # when we drilled away, so navigating straight back would re-LIST + # into an empty flash. Any parent row is enough to render. + prewarm_scope = pane.scope + await self._prewarm_view(peeked.parent_kind, prewarm_scope, lambda rows: bool(rows)) + try: + async with self._nav_lock: + if pane not in self._panes: + return False # the initiating pane was closed while queued + popped = pane.drill.pop() + if popped is None: + return False + await self._navigate_locked(pane, popped.parent_kind, None) + finally: + await self._stop_watch_if_unused(peeked.parent_kind, prewarm_scope) self._render_table(pane.kind, only=pane) self._refresh_status() return True diff --git a/src/korvid/ui/navigation.py b/src/korvid/ui/navigation.py index 5b315954..44f65bf2 100644 --- a/src/korvid/ui/navigation.py +++ b/src/korvid/ui/navigation.py @@ -51,6 +51,10 @@ def pop(self) -> DrillLevel | None: """Remove the top level; the popped parent_kind is the view to show.""" return self._levels.pop() if self._levels else None + def peek(self) -> DrillLevel | None: + """The top level without removing it; None when not drilled.""" + return self._levels[-1] if self._levels else None + def clear(self) -> None: self._levels.clear() diff --git a/tests/ui/test_drilldown.py b/tests/ui/test_drilldown.py index e9c55b5a..4c31a5bf 100644 --- a/tests/ui/test_drilldown.py +++ b/tests/ui/test_drilldown.py @@ -8,11 +8,14 @@ from korvid.core.watch import WatchManager from korvid.k8s.discovery import ResourceMeta from korvid.k8s.models import GenericSummary, PodSummary, ReplicaSetSummary +from korvid.k8s.relations import owned_by from korvid.ui.app import KorvidApp from korvid.ui.messages import FilterCommand, NavigateCommand from korvid.ui.widgets.resource_table import ResourceTable from korvid.ui.widgets.status_bar import StatusBar +from .waits import until + _PODS_META = ResourceMeta("Pod", "pods", "", "v1", True, ("po",)) _DEPLOY_META = ResourceMeta("Deployment", "deployments", "apps", "v1", True, ("deploy",)) _RS_META = ResourceMeta("ReplicaSet", "replicasets", "apps", "v1", True, ("rs",)) @@ -348,15 +351,19 @@ async def test_concurrent_drill_and_navigate_stay_consistent() -> None: await pilot.pause(0.1) await _navigate(pilot, "deployments") gate = asyncio.Event() + entered = asyncio.Event() orig_stop = app.watch_manager.stop async def slow_stop(kind: str, scope: str) -> None: + entered.set() await gate.wait() await orig_stop(kind, scope) app.watch_manager.stop = slow_stop # type: ignore[method-assign] # test seam to widen the race window drill = asyncio.create_task(app.agent_drill_down("web")) - await asyncio.sleep(0.02) # drill enters the lock and blocks in stop() + # The drill pre-warms before taking the lock (issue #157): wait until + # it is really inside the critical section, blocked in stop(). + await asyncio.wait_for(entered.wait(), timeout=5) nav = asyncio.create_task(app.on_navigate_command(NavigateCommand("pods", None))) await asyncio.sleep(0.02) gate.set() @@ -371,3 +378,154 @@ async def slow_stop(kind: str, scope: str) -> None: assert table.row_count == 2 status = str(app.query_one(StatusBar).content) assert "deployments/" not in status + + +# --------------------------------------------------------------------------- +# drill pre-warm (issue #157): no empty-view flash between push/pop and rows +# --------------------------------------------------------------------------- + + +def _make_slow_app( + data: dict[str, list[Summary]], + *, + delay_kinds: dict[str, float], + starts: list[str] | None = None, +) -> KorvidApp: + """App whose watch source stalls before LISTing `delay_kinds[kind]` + seconds - a stand-in for the network RTT that produced the empty flash.""" + store = ResourceStore() + + async def source(kind: str, scope: str) -> AsyncIterator[tuple[str, Summary]]: + if starts is not None: + starts.append(kind) + delay = delay_kinds.get(kind, 0.0) + if delay: + await asyncio.sleep(delay) + for obj in data.get(kind, []): + yield ("ADDED", obj) + while True: + await asyncio.sleep(0.01) + + async def list_namespaces() -> list[str]: + return ["default"] + + return KorvidApp( + config=KorvidConfig(namespace="default"), + store=store, + watch_manager=WatchManager(store, source), + list_namespaces=list_namespaces, + aliases=dict(_ALIASES), + ) + + +def _spy_renders(app: KorvidApp, renders: list[tuple[str, int]]) -> None: + original = type(app)._render_pane + + def spy(self, kind, pane, table, *, empty_state): # type: ignore[no-untyped-def] # test seam + rows = self.store.get(kind, pane.scope) + drill_uid = pane.drill.parent_uid + if drill_uid is not None and kind == pane.drill.child_kind: + rows = [r for r in rows if owned_by(r, drill_uid)] + renders.append((kind, len(rows))) + original(self, kind, pane, table, empty_state=empty_state) + + app._render_pane = spy.__get__(app) # type: ignore[method-assign] # test seam + + +async def test_drill_push_never_renders_an_empty_child_view() -> None: + """The old flow switched the pane first and LISTed after: one visibly + empty replicasets render, then the fill. The pre-warm starts the child + watch before the switch, so the first child render already has rows.""" + renders: list[tuple[str, int]] = [] + app = _make_slow_app(_default_data(), delay_kinds={"replicasets": 0.15}) + async with app.run_test() as pilot: + await pilot.pause(0.1) + await _navigate(pilot, "deployments") + _spy_renders(app, renders) + await pilot.press("down") # api -> web + await pilot.press("enter") + await until(pilot, lambda: app.current_kind == "replicasets", label="drilled") + table = app.query_one(ResourceTable) + await until(pilot, lambda: table.row_count == 2, label="owned rows visible") + assert ("replicasets", 0) not in renders + + +async def test_drill_pop_never_renders_an_empty_parent_view() -> None: + """Esc re-LISTs the parent kind (its watch stopped when we drilled + away): the pre-warm must cover the pop direction too.""" + delays = {"deployments": 0.0} + renders: list[tuple[str, int]] = [] + app = _make_slow_app(_default_data(), delay_kinds=delays) + async with app.run_test() as pilot: + await pilot.pause(0.1) + await _navigate(pilot, "deployments") + await pilot.press("down") + await pilot.press("enter") + await until(pilot, lambda: app.current_kind == "replicasets", label="drilled") + delays["deployments"] = 0.15 # the re-LIST on the way back is slow + _spy_renders(app, renders) + await pilot.press("escape") + await until(pilot, lambda: app.current_kind == "deployments", label="popped") + table = app.query_one(ResourceTable) + await until(pilot, lambda: table.row_count == 2, label="parents visible") + assert ("deployments", 0) not in renders + + +async def test_drill_prewarm_shows_a_progress_label_while_waiting() -> None: + """The bounded wait must read as *working*, not frozen: the status bar + carries a loading label (which the corvid busy indicator animates).""" + app = _make_slow_app(_default_data(), delay_kinds={"replicasets": 0.3}) + async with app.run_test() as pilot: + await pilot.pause(0.1) + await _navigate(pilot, "deployments") + # Drive the drill as a task: pilot.press would await the whole + # transition, leaving no window to observe the in-flight label. + drill = asyncio.create_task(app._drill_into("default", "web")) + await until( + pilot, + lambda: any("replicasets" in v for v in app._progress_labels.values()), + label="loading label published", + ) + assert app.current_kind == "deployments" # still on the parent view + assert (await drill) is None + assert app.current_kind == "replicasets" + assert not app._progress_labels # cleared once the switch landed + + +async def test_drill_prewarm_times_out_and_still_switches() -> None: + """A cluster that never answers must not wedge the drill: after the + bounded wait the transition proceeds exactly as before the pre-warm.""" + data = _default_data() + data["replicasets"] = [] # LIST returns nothing to own + app = _make_slow_app(data, delay_kinds={}) + app.DRILL_PREWARM_TIMEOUT = 0.1 + async with app.run_test() as pilot: + await pilot.pause(0.1) + await _navigate(pilot, "deployments") + await pilot.press("down") + await pilot.press("enter") + await until(pilot, lambda: app.current_kind == "replicasets", label="switched anyway") + table = app.query_one(ResourceTable) + assert table.row_count == 0 # genuinely empty child view is correct + + +async def test_drill_prewarm_skips_the_wait_when_the_watch_is_live() -> None: + """A (kind, scope) another pane already watches has a warm bucket: the + drill must not re-clear it or wait.""" + starts: list[str] = [] + app = _make_slow_app(_default_data(), delay_kinds={}, starts=starts) + async with app.run_test() as pilot: + await pilot.pause(0.1) + await _navigate(pilot, "deployments") + await pilot.press("ctrl+w") + await pilot.press("v") # split: both panes on deployments + await pilot.pause(0.1) + await _navigate(pilot, "replicasets") # focused pane -> rs watch live + await pilot.press("ctrl+w") + await pilot.press("w") # focus back to the deployments pane + await pilot.pause(0.1) + starts.clear() + await pilot.press("down") + await pilot.press("enter") + await until(pilot, lambda: app.current_kind == "replicasets", label="drilled") + assert "replicasets" not in starts # live watch reused, not restarted From ec3ea6f0ae549d3c4ee56031bd30a8e6a23ae8bc Mon Sep 17 00:00:00 2001 From: hellices Date: Mon, 3 Aug 2026 01:01:45 +0900 Subject: [PATCH 2/4] fix: abandon stale drills - a newer navigation or ctx switch wins Review round 1 on #160: the pre-warm widened the window between the drill's intent and its locked transaction to up to a second, so a newer :view/:ns or a context switch landing meanwhile could be overridden by the stale drill (or worse, an old-cluster UID applied after the epoch changed). Both _drill_into and _pop_drill now anchor (kind, scope) and _ctx_epoch before the pre-warm and revalidate under the lock - a mismatch abandons the drill with an accurate result string (never a false 'drilled into ...' success for the agent), and the pop side also requires the peeked level to still be the top of the stack. Abandoned streams are reaped by the existing _stop_watch_if_unused finally. Tests: test_drill_abandons_when_a_newer_navigation_lands_during_prewarm, test_drill_abandons_across_a_context_epoch_change, test_pop_abandons_when_the_view_changed_during_prewarm. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/korvid/ui/app.py | 29 ++++++++++++++++ tests/ui/test_drilldown.py | 70 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+) diff --git a/src/korvid/ui/app.py b/src/korvid/ui/app.py index e3082283..71ed59ed 100644 --- a/src/korvid/ui/app.py +++ b/src/korvid/ui/app.py @@ -2248,6 +2248,12 @@ async def _drill_into(self, namespace: str, name: str) -> str | None: # Capture before waiting on the lock: focus may move (or the pane may # close) while this drill queues behind another navigation. pane = self._pane + # Staleness anchors (review on #160): the pre-warm below can wait up + # to a second, so a newer :view/:ns/:ctx may land first. The drill + # was issued against *this* view in *this* cluster - anything else + # under the lock means the newer command wins and the drill abandons. + origin = (pane.kind, pane.scope) + epoch = self._ctx_epoch # Warm the child view first (issue #157): wait - bounded - until the # rows this drill will show exist, so the switch renders once with # content instead of flashing an empty table while the LIST runs. @@ -2261,6 +2267,15 @@ async def _drill_into(self, namespace: str, name: str) -> str | None: async with self._nav_lock: if pane not in self._panes: return None # the initiating pane was closed while queued + if ( + (pane.kind, pane.scope) != origin + or self._ctx_switching + or epoch != self._ctx_epoch + ): + return ( + "the view changed while preparing the drill — drill abandoned " + "(the newer navigation takes priority)" + ) pane.drill.push(level) try: await self._navigate_locked(pane, child, None) @@ -2286,6 +2301,10 @@ async def _pop_drill(self) -> bool: peeked = pane.drill.peek() if peeked is None: return False + # Staleness anchors (review on #160): same rule as the push side - + # the Esc was issued against this view in this cluster. + origin = (pane.kind, pane.scope) + epoch = self._ctx_epoch # Warm the parent view first (issue #157): its watch was stopped # when we drilled away, so navigating straight back would re-LIST # into an empty flash. Any parent row is enough to render. @@ -2295,6 +2314,16 @@ async def _pop_drill(self) -> bool: async with self._nav_lock: if pane not in self._panes: return False # the initiating pane was closed while queued + if ( + (pane.kind, pane.scope) != origin + or self._ctx_switching + or epoch != self._ctx_epoch + or pane.drill.peek() is not peeked + ): + # A newer navigation landed during the pre-warm: it wins. + # Consume the Esc (True) so it does not cascade into the + # hierarchy-return fallback against the changed view. + return True popped = pane.drill.pop() if popped is None: return False diff --git a/tests/ui/test_drilldown.py b/tests/ui/test_drilldown.py index 4c31a5bf..bfbeb42d 100644 --- a/tests/ui/test_drilldown.py +++ b/tests/ui/test_drilldown.py @@ -529,3 +529,73 @@ async def test_drill_prewarm_skips_the_wait_when_the_watch_is_live() -> None: await pilot.press("enter") await until(pilot, lambda: app.current_kind == "replicasets", label="drilled") assert "replicasets" not in starts # live watch reused, not restarted + + +async def test_drill_abandons_when_a_newer_navigation_lands_during_prewarm() -> None: + """The pre-warm widens the window between Enter and the locked + transaction: a `:view` issued meanwhile is the newer command and must + win - the stale drill must not override it or strand a level.""" + app = _make_slow_app(_default_data(), delay_kinds={"replicasets": 0.3}) + async with app.run_test() as pilot: + await pilot.pause(0.1) + await _navigate(pilot, "deployments") + drill = asyncio.create_task(app._drill_into("default", "web")) + await until( + pilot, + lambda: ("replicasets", "default") in app.watch_manager.active, + label="prewarm started", + ) + await app.on_navigate_command(NavigateCommand("pods", None)) + result = await drill + assert result is not None # an accurate outcome, not a false success + assert "abandoned" in result + await pilot.pause(0.1) + assert app.current_kind == "pods" # the newer command won + assert not app._pane.drill.active # no stranded drill level + # the pre-warmed replicasets stream was reaped, not leaked + assert ("replicasets", "default") not in app.watch_manager.active + + +async def test_drill_abandons_across_a_context_epoch_change() -> None: + """A context switch during the pre-warm invalidates the captured UID + (it names an object in the old cluster): the drill must abandon.""" + app = _make_slow_app(_default_data(), delay_kinds={"replicasets": 0.3}) + async with app.run_test() as pilot: + await pilot.pause(0.1) + await _navigate(pilot, "deployments") + drill = asyncio.create_task(app._drill_into("default", "web")) + await until( + pilot, + lambda: ("replicasets", "default") in app.watch_manager.active, + label="prewarm started", + ) + app._ctx_epoch += 1 # what a :ctx switch does + result = await drill + assert result is not None + assert "abandoned" in result + assert app.current_kind == "deployments" # stayed put + assert not app._pane.drill.active + + +async def test_pop_abandons_when_the_view_changed_during_prewarm() -> None: + """Esc's pop pre-warms the parent kind: a navigation landing during + that wait cleared the drill stack - the stale pop must not navigate.""" + delays = {"deployments": 0.0} + app = _make_slow_app(_default_data(), delay_kinds=delays) + async with app.run_test() as pilot: + await pilot.pause(0.1) + await _navigate(pilot, "deployments") + await pilot.press("down") + await pilot.press("enter") + await until(pilot, lambda: app.current_kind == "replicasets", label="drilled") + delays["deployments"] = 0.3 # slow re-LIST on the way back + pop = asyncio.create_task(app._pop_drill()) + await until( + pilot, + lambda: ("deployments", "default") in app.watch_manager.active, + label="pop prewarm started", + ) + await app.on_navigate_command(NavigateCommand("pods", None)) + assert await pop is True # consumed, but did not override + await pilot.pause(0.1) + assert app.current_kind == "pods" # the newer command won From cc144e67d457cbc0ec9d8c6a07369abf934def19 Mon Sep 17 00:00:00 2001 From: hellices Date: Mon, 3 Aug 2026 01:25:50 +0900 Subject: [PATCH 3/4] fix: nav generation catches same-target commands; lease-counted prewarms Review round 2 on #160 (all three suppressed findings credible): - a ':view deployments' issued while already on deployments is still the newer command (it clears drill state) but left the (kind, scope) tuple and epoch unchanged - the stale drill pushed over it. Every _navigate_locked call now advances a per-pane nav_gen, and both drill paths capture + revalidate it under the lock (test_drill_abandons_when_a_same_target_navigation_lands_during_prewarm). - a pane closed during the pre-warm returned None, which agent_drill_down reports as a successful drill with a breadcrumb - now an accurate abandonment string (test_pane_closed_during_prewarm_reports_abandonment). - watch_manager.active includes another drill's in-flight pre-warm, so an overlapping drill skipped its wait and recreated the empty flash, and one drill's cleanup could stop the stream the other relied on. _prewarm_view skips only pane-backed watches, always waits on the caller's own readiness, and _stop_watch_if_unused is lease-counted: only the last release may reap an undisplayed stream (test_overlapping_drills_do_not_skip_each_others_prewarm). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/korvid/ui/app.py | 51 +++++++++++++++++++++---- tests/ui/test_drilldown.py | 78 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+), 7 deletions(-) diff --git a/src/korvid/ui/app.py b/src/korvid/ui/app.py index 71ed59ed..0fb1a3c6 100644 --- a/src/korvid/ui/app.py +++ b/src/korvid/ui/app.py @@ -511,6 +511,12 @@ def __init__(self, kind: str, scope: str, table_id: str = "pane-0") -> None: self.filter_pattern = "" self.resource_filter: ResourceFilter = parse_filter("") self.drill = NavigationStack() + #: Monotonic navigation counter: every _navigate_locked call on this + #: pane advances it, including same-target ones. A drill pre-warm + #: (issue #157) captures it before waiting and revalidates under the + #: lock - a `:view deployments` while already on deployments is + #: still the newer command and must not be overridden. + self.nav_gen = 0 #: Pending way back to a hierarchy tree a goto jump navigated away #: from (issue #135); consumed by Escape in this pane. View state #: like the drill stack - never shared across panes. @@ -918,6 +924,10 @@ def __init__( # Kinds with a table render already queued — coalesces the per-object # notifications of a LIST seed into a single rebuild (see _on_store_update). self._render_pending: set[str] = set() + #: Outstanding drill pre-warm leases per (kind, scope) (issue #157): + #: overlapping drills each hold one; only the last release may reap + #: a stream no pane displays. + self._prewarm_leases: dict[tuple[str, str], int] = {} # Rebuild inputs for an open HierarchyScreen: (title, refs, namespace, # scope). Store updates rebuild the tree in place while it is open. self._hierarchy_ctx: tuple[str, list[ComponentRef], str, str] | None = None @@ -1464,6 +1474,10 @@ async def _navigate_locked( self, pane: PaneState, view: str | None, namespace: str | None ) -> None: """Kind/scope transition body; caller must hold ``_nav_lock``.""" + # Advance the pane's navigation generation first: a queued drill + # revalidating after its pre-warm must observe this command even + # when the kind/scope tuple ends up unchanged. + pane.nav_gen += 1 # A describe pane covering the table would show a stale manifest # over the new view — dismiss it on any navigation, even when the # requested kind/scope already matches. @@ -2193,10 +2207,18 @@ async def _prewarm_view( subsequent `_navigate_locked` start() is then a no-op (the watch is already running), the bucket is warm, and the single post-switch render lands with real rows instead of flashing an empty table. - A live watch (another pane shows this kind/scope) is already warm - - restarting it would clear the bucket it serves. + + A pane-backed watch (a split pane displays this kind/scope) is + already warm - restarting it would clear the bucket it serves, so + both restart and wait are skipped. A watch that is merely *active* + may be another drill's in-flight pre-warm whose LIST has not landed: + each caller waits on its own readiness, and the lease count makes + `_stop_watch_if_unused` reap the stream only when the last pre-warm + released it. """ - if (kind, scope) in self.watch_manager.active: + key = (kind, scope) + self._prewarm_leases[key] = self._prewarm_leases.get(key, 0) + 1 + if any((p.kind, p.scope) == key for p in self._panes): return await self.watch_manager.start(kind, scope) deadline = monotonic() + self.DRILL_PREWARM_TIMEOUT @@ -2207,9 +2229,17 @@ async def _prewarm_view( await asyncio.sleep(0.03) async def _stop_watch_if_unused(self, kind: str, scope: str) -> None: - """Reap a pre-warmed watch no pane ended up displaying (issue #157): - a drill that lost its pane (or its race) must not leak a stream.""" - if all((p.kind, p.scope) != (kind, scope) for p in self._panes): + """Release one pre-warm lease; reap the stream when it was the last + lease and no pane displays the (kind, scope) (issue #157): a drill + that lost its pane (or its race) must not leak a watch, and must + not stop one a concurrent pre-warm or pane still relies on.""" + key = (kind, scope) + remaining = self._prewarm_leases.get(key, 0) - 1 + if remaining > 0: + self._prewarm_leases[key] = remaining + return + self._prewarm_leases.pop(key, None) + if all((p.kind, p.scope) != key for p in self._panes): await self.watch_manager.stop(kind, scope) async def _drill_into(self, namespace: str, name: str) -> str | None: @@ -2254,6 +2284,7 @@ async def _drill_into(self, namespace: str, name: str) -> str | None: # under the lock means the newer command wins and the drill abandons. origin = (pane.kind, pane.scope) epoch = self._ctx_epoch + nav_gen = pane.nav_gen # Warm the child view first (issue #157): wait - bounded - until the # rows this drill will show exist, so the switch renders once with # content instead of flashing an empty table while the LIST runs. @@ -2266,9 +2297,13 @@ async def _drill_into(self, namespace: str, name: str) -> str | None: try: async with self._nav_lock: if pane not in self._panes: - return None # the initiating pane was closed while queued + # An accurate outcome (review on #160): a None here reads + # as success to agent_drill_down, which would report a + # drill that never happened. + return "the pane closed while preparing the drill — drill abandoned" if ( (pane.kind, pane.scope) != origin + or pane.nav_gen != nav_gen or self._ctx_switching or epoch != self._ctx_epoch ): @@ -2305,6 +2340,7 @@ async def _pop_drill(self) -> bool: # the Esc was issued against this view in this cluster. origin = (pane.kind, pane.scope) epoch = self._ctx_epoch + nav_gen = pane.nav_gen # Warm the parent view first (issue #157): its watch was stopped # when we drilled away, so navigating straight back would re-LIST # into an empty flash. Any parent row is enough to render. @@ -2316,6 +2352,7 @@ async def _pop_drill(self) -> bool: return False # the initiating pane was closed while queued if ( (pane.kind, pane.scope) != origin + or pane.nav_gen != nav_gen or self._ctx_switching or epoch != self._ctx_epoch or pane.drill.peek() is not peeked diff --git a/tests/ui/test_drilldown.py b/tests/ui/test_drilldown.py index bfbeb42d..28346baf 100644 --- a/tests/ui/test_drilldown.py +++ b/tests/ui/test_drilldown.py @@ -599,3 +599,81 @@ async def test_pop_abandons_when_the_view_changed_during_prewarm() -> None: assert await pop is True # consumed, but did not override await pilot.pause(0.1) assert app.current_kind == "pods" # the newer command won + + +async def test_drill_abandons_when_a_same_target_navigation_lands_during_prewarm() -> None: + """`:view deployments` while already on deployments is still the newer + command (it clears drill state): a (kind, scope) tuple comparison alone + cannot see it - the per-pane navigation generation must.""" + app = _make_slow_app(_default_data(), delay_kinds={"replicasets": 0.3}) + async with app.run_test() as pilot: + await pilot.pause(0.1) + await _navigate(pilot, "deployments") + drill = asyncio.create_task(app._drill_into("default", "web")) + await until( + pilot, + lambda: ("replicasets", "default") in app.watch_manager.active, + label="prewarm started", + ) + await app.on_navigate_command(NavigateCommand("deployments", None)) # same target + result = await drill + assert result is not None + assert "abandoned" in result + assert app.current_kind == "deployments" + assert not app._pane.drill.active # the newer command's clear stands + + +async def test_pane_closed_during_prewarm_reports_abandonment() -> None: + """agent_drill_down reports success on a None result: a pane closed + during the pre-warm must yield an accurate abandonment, not a false + 'drilled into ...' with a breadcrumb.""" + app = _make_slow_app(_default_data(), delay_kinds={"replicasets": 0.3}) + async with app.run_test() as pilot: + await pilot.pause(0.1) + await _navigate(pilot, "deployments") + await pilot.press("ctrl+w") + await pilot.press("v") # split so a pane *can* close + await pilot.pause(0.1) + drill = asyncio.create_task(app._drill_into("default", "web")) + await until( + pilot, + lambda: ("replicasets", "default") in app.watch_manager.active, + label="prewarm started", + ) + await pilot.press("ctrl+w") + await pilot.press("q") # close the initiating pane mid-wait + result = await drill + assert result is not None + assert "abandoned" in result + # the pre-warmed stream was reaped, not leaked + await until( + pilot, + lambda: ("replicasets", "default") not in app.watch_manager.active, + label="prewarm reaped", + ) + + +async def test_overlapping_drills_do_not_skip_each_others_prewarm() -> None: + """Two drills racing to the same (kind, scope): the second must wait on + its *own* readiness instead of treating the first's in-flight pre-warm + watch as warm - skipping recreated the empty-view flash.""" + renders: list[tuple[str, int]] = [] + app = _make_slow_app(_default_data(), delay_kinds={"replicasets": 0.25}) + async with app.run_test() as pilot: + await pilot.pause(0.1) + await _navigate(pilot, "deployments") + _spy_renders(app, renders) + first = asyncio.create_task(app._drill_into("default", "web")) + await until( + pilot, + lambda: ("replicasets", "default") in app.watch_manager.active, + label="first prewarm started", + ) + second = asyncio.create_task(app._drill_into("default", "api")) + results = [await first, await second] + await pilot.pause(0.1) + assert app.current_kind == "replicasets" + assert ("replicasets", 0) not in renders # neither drill flashed empty + # exactly one drill landed; the loser abandoned with an accurate result + assert sum(1 for r in results if r is None) == 1 + assert any(r is not None and "abandoned" in r for r in results) From f1b4ad430b653ef2a5b11cb1c6b7495fd5f0f6c3 Mon Sep 17 00:00:00 2001 From: hellices Date: Mon, 3 Aug 2026 01:50:30 +0900 Subject: [PATCH 4/4] fix: leak-proof prewarm leases; readiness follows the post-pop filter Review round 3 on #160: - the lease acquire sat outside the try: a drill task cancelled mid-pre-warm (:ctx teardown) left a permanent lease - blocking every later reap on that (kind, scope) - and leaked the started watch. The pre-warm call moved inside the try; the acquire is synchronous before the first await, so the finally never releases a lease that was not taken (test_cancelled_prewarm_releases_its_lease_and_watch). - the pane-backed fast path raced watch teardown: a pane mid-navigate keeps its tuple while stop() has already removed the stream. The fast path now also requires the watch to be live, and _navigate_locked's teardown skips streams with outstanding pre-warm leases - the last lease release reaps them (test_prewarm_restarts_a_dead_watch_even_when_pane_backed, test_navigation_teardown_honors_outstanding_prewarm_leases). - popping pods -> replicasets keeps the deployment-UID filter, but the readiness accepted any row: an unrelated ReplicaSet arriving first switched into a zero-row filtered view. Readiness now mirrors what the post-pop view will show - owned rows for a remaining level, any row only at the root (test_two_level_pop_waits_for_rows_the_drill_filter_will_show). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/korvid/ui/app.py | 43 +++++++++++---- tests/ui/test_drilldown.py | 105 +++++++++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 9 deletions(-) diff --git a/src/korvid/ui/app.py b/src/korvid/ui/app.py index 0fb1a3c6..b69c8a2f 100644 --- a/src/korvid/ui/app.py +++ b/src/korvid/ui/app.py @@ -1493,7 +1493,11 @@ async def _navigate_locked( # Another pane may still be watching the old (kind, scope): # stopping it would freeze that pane's view (issue #48). others = {(p.kind, p.scope) for p in self._panes if p is not pane} - if old not in others: + if old not in others and self._prewarm_leases.get(old, 0) == 0: + # An outstanding drill pre-warm lease keeps the stream alive + # (issue #157): killing it here would force that drill's own + # navigate to re-LIST into the empty flash. The last lease + # release reaps it if no pane ends up displaying it. await self.watch_manager.stop(*old) pane.kind = new_kind pane.scope = new_scope @@ -2218,7 +2222,11 @@ async def _prewarm_view( """ key = (kind, scope) self._prewarm_leases[key] = self._prewarm_leases.get(key, 0) + 1 - if any((p.kind, p.scope) == key for p in self._panes): + # Pane-backed *and* live: a pane's watch mid-teardown (a concurrent + # navigation awaiting stop()) leaves the pane tuple unchanged while + # the stream is already gone - skipping then would recreate the + # empty flash. Require the watch itself. + if any((p.kind, p.scope) == key for p in self._panes) and key in self.watch_manager.active: return await self.watch_manager.start(kind, scope) deadline = monotonic() + self.DRILL_PREWARM_TIMEOUT @@ -2288,13 +2296,16 @@ async def _drill_into(self, namespace: str, name: str) -> str | None: # Warm the child view first (issue #157): wait - bounded - until the # rows this drill will show exist, so the switch renders once with # content instead of flashing an empty table while the LIST runs. + # Inside the try: a cancellation mid-pre-warm must still release the + # lease (the acquire is synchronous before the first await, so the + # finally never releases a lease that was not taken). prewarm_scope = pane.scope - await self._prewarm_view( - child, - prewarm_scope, - lambda rows: any(owned_by(r, uid) for r in rows), - ) try: + await self._prewarm_view( + child, + prewarm_scope, + lambda rows: any(owned_by(r, uid) for r in rows), + ) async with self._nav_lock: if pane not in self._panes: # An accurate outcome (review on #160): a None here reads @@ -2343,10 +2354,24 @@ async def _pop_drill(self) -> bool: nav_gen = pane.nav_gen # Warm the parent view first (issue #157): its watch was stopped # when we drilled away, so navigating straight back would re-LIST - # into an empty flash. Any parent row is enough to render. + # into an empty flash. Readiness is what the post-pop view will + # actually show: a remaining drill level keeps filtering by its + # parent UID (pods -> replicasets keeps the deployment filter), so + # an unrelated row must not satisfy the wait; only a pop back to + # the root accepts any row. + under = pane.drill.copy() + under.pop() + uid_after = under.parent_uid + if uid_after is None: + ready: Callable[[list[Summary]], bool] = bool + else: + + def ready(rows: list[Summary]) -> bool: + return any(owned_by(r, uid_after) for r in rows) + prewarm_scope = pane.scope - await self._prewarm_view(peeked.parent_kind, prewarm_scope, lambda rows: bool(rows)) try: + await self._prewarm_view(peeked.parent_kind, prewarm_scope, ready) async with self._nav_lock: if pane not in self._panes: return False # the initiating pane was closed while queued diff --git a/tests/ui/test_drilldown.py b/tests/ui/test_drilldown.py index 28346baf..3cfb6a54 100644 --- a/tests/ui/test_drilldown.py +++ b/tests/ui/test_drilldown.py @@ -3,6 +3,8 @@ import asyncio from collections.abc import AsyncIterator +import pytest + from korvid.core.config import KorvidConfig from korvid.core.store import ResourceStore, Summary from korvid.core.watch import WatchManager @@ -677,3 +679,106 @@ async def test_overlapping_drills_do_not_skip_each_others_prewarm() -> None: # exactly one drill landed; the loser abandoned with an accurate result assert sum(1 for r in results if r is None) == 1 assert any(r is not None and "abandoned" in r for r in results) + + +async def test_cancelled_prewarm_releases_its_lease_and_watch() -> None: + """A drill task cancelled mid-pre-warm (e.g. app teardown, :ctx) must + not leave a permanent lease - that would block stream reaping for every + later drill on the same (kind, scope) - nor leak the started watch.""" + app = _make_slow_app(_default_data(), delay_kinds={"replicasets": 5.0}) + async with app.run_test() as pilot: + await pilot.pause(0.1) + await _navigate(pilot, "deployments") + drill = asyncio.create_task(app._drill_into("default", "web")) + await until( + pilot, + lambda: ("replicasets", "default") in app.watch_manager.active, + label="prewarm started", + ) + drill.cancel() + with pytest.raises(asyncio.CancelledError): + await drill + await until( + pilot, + lambda: not app._prewarm_leases, + label="lease released", + ) + assert ("replicasets", "default") not in app.watch_manager.active + + +async def test_two_level_pop_waits_for_rows_the_drill_filter_will_show() -> None: + """Popping pods -> replicasets keeps the deployment-UID filter: an + unrelated ReplicaSet arriving first must not satisfy the readiness and + flash a zero-row filtered view.""" + data = _default_data() + store = ResourceStore() + rs_lists = {"n": 0} + + async def source(kind: str, scope: str) -> AsyncIterator[tuple[str, Summary]]: + if kind == "replicasets": + rs_lists["n"] += 1 + if rs_lists["n"] > 1: # the re-LIST on the way back + yield ("ADDED", _rs("api-777", "rs-9", "dep-9")) # unrelated first + await asyncio.sleep(0.2) + for obj in data.get(kind, []): + yield ("ADDED", obj) + while True: + await asyncio.sleep(0.01) + + async def list_namespaces() -> list[str]: + return ["default"] + + app = KorvidApp( + config=KorvidConfig(namespace="default"), + store=store, + watch_manager=WatchManager(store, source), + list_namespaces=list_namespaces, + aliases=dict(_ALIASES), + ) + renders: list[tuple[str, int]] = [] + async with app.run_test() as pilot: + await pilot.pause(0.1) + await _navigate(pilot, "deployments") + await pilot.press("down") + await pilot.press("enter") # web -> replicasets + await until(pilot, lambda: app.current_kind == "replicasets", label="rs level") + await pilot.press("enter") # -> pods + await until(pilot, lambda: app.current_kind == "pods", label="pods level") + _spy_renders(app, renders) + await pilot.press("escape") + await until(pilot, lambda: app.current_kind == "replicasets", label="popped to rs") + table = app.query_one(ResourceTable) + await until(pilot, lambda: table.row_count == 2, label="owned rows visible") + assert ("replicasets", 0) not in renders + + +async def test_prewarm_restarts_a_dead_watch_even_when_pane_backed() -> None: + """A pane displaying the target is only warm while its watch lives: a + teardown racing the check must not skip the start and the wait.""" + app = make_app(_default_data()) + async with app.run_test() as pilot: + await pilot.pause(0.1) + await _navigate(pilot, "replicasets") + await app.watch_manager.stop("replicasets", "default") # teardown race stand-in + await app._prewarm_view("replicasets", "default", lambda rows: bool(rows)) + assert ("replicasets", "default") in app.watch_manager.active # restarted + await app._stop_watch_if_unused("replicasets", "default") + # still displayed by the pane: the release must not reap it + assert ("replicasets", "default") in app.watch_manager.active + + +async def test_navigation_teardown_honors_outstanding_prewarm_leases() -> None: + """_navigate_locked stops the view it leaves - unless a drill pre-warm + still holds a lease on that stream; killing it would force the drill's + own navigate to re-LIST into the empty flash.""" + app = make_app(_default_data()) + async with app.run_test() as pilot: + await pilot.pause(0.1) + await _navigate(pilot, "replicasets") + app._prewarm_leases[("replicasets", "default")] = 1 # an in-flight drill's lease + await app.on_navigate_command(NavigateCommand("pods", None)) + await pilot.pause(0.1) + assert app.current_kind == "pods" + assert ("replicasets", "default") in app.watch_manager.active # lease honored + await app._stop_watch_if_unused("replicasets", "default") # last release + assert ("replicasets", "default") not in app.watch_manager.active # now reaped