perf: memoise table rows so a repaint costs only what changed - #209
Merged
Conversation
Every watch event triggered a full-table repaint whose cost was proportional to the total row count, not to the number of objects that changed, and it ran synchronously on the event loop — so keypresses queued behind it. At 10,000 objects a single repaint cost ~170 ms and 10 events/s pushed cursor movement past 2.5 s; at 50,000 objects one repaint cost ~1.1 s. Watch summaries are frozen dataclasses replaced wholesale, so a row whose summary object is unchanged provably renders identical cells. Four changes: - `ResourceTable._row_memo` keyed by summary identity plus a `stamp` covering the inputs that are *not* carried by the summary (the age string, and the metrics sample on the pods view). A hit re-emits the memoised row untouched. The memo is invalidated when the cell-set shape changes (kind, all-namespaces, custom view) and pruned amortised as row names churn. - The in-place diff compares against `_emitted` — the cells this widget last put into the table — instead of `DataTable.get_row()`, which re-derived `ordered_columns` on every call. A memo hit is settled by one identity check; `get_row` stays as the fallback when `_emitted` has no record. - One `datetime.now(UTC)` per repaint threaded into every `age()` call, replacing a clock read plus RFC 3339 reparse per row per repaint. Rows in one repaint can no longer straddle a second boundary either. - Cell builders skip rebuilding memo-hit rows entirely. Measured on the same fixtures as issue #208 (pods, all-namespaces): | rows | repaint before | repaint after | cell build before | after | |--------|----------------|---------------|-------------------|---------| | 1,000 | 11.40 ms | 1.04 ms | 6.34 ms | 0.63 ms| | 10,000 | 170.05 ms | 11.46 ms | 139.17 ms | 7.05 ms| | 50,000 | 1,102.36 ms | 80.12 ms | 780.61 ms | 42.82 ms| Keypress latency under churn (30 samples, cursor-down), 10 events/s: | rows | before | after | |--------|----------|---------| | 10,000 | 2,551 ms | 100 ms| | 50,000 | 3,364 ms | 2,178 ms| Cold first build is unchanged (it has no memo to hit) and remains the dominant cost at 50,000 rows; that is a separate bootstrap problem. Fixes #208 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Optimizes resource-table repaints by reusing unchanged rows and avoiding costly DataTable reads.
Changes:
- Memoizes rendered rows using summary identity and volatile inputs.
- Tracks emitted cells for efficient in-place updates.
- Adds behavioral regression tests for memoization, metrics, and clock consistency.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/korvid/ui/widgets/resource_table.py |
Implements row memoization and optimized diffing. |
tests/ui/test_table_row_memo.py |
Tests row reuse and invalidation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review of #209: with a `replace: true` custom view (issue #45), `_emit_row` keeps only NAME plus the configured custom values — both carried by the frozen summary — and discards the built-in AGE and metrics cells. The stamp still carried them, so a minute rollover or a metrics poll invalidated the memo and rebuilt cells nobody can see. Route every stamp through `_stamp()`, which collapses to `None` under a replace view because nothing volatile survives into the emitted row. Test: `test_replace_view_rows_ignore_hidden_volatile_cells` configures a replace view, publishes a new metrics sample for an unchanged pod object, and asserts no row is rebuilt while the rendered row stays `[NAME, TEAM]`. RED against 45da6a5 (the row rebuilt), GREEN now. Repaint cost is unchanged: 0.95 / 11.21 / 82.15 ms at 1k / 10k / 50k. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/ui/test_table_row_memo.py:157
- The clock test deliberately uses fresh summary objects, so it bypasses the memo and would still pass if AGE were removed from the stamp. That regression would freeze AGE cells indefinitely for unchanged resources. Please also keep the same summary identity across two renders, advance the clock across an age boundary, and assert that the AGE cell is updated.
# Fresh summary objects: identity differs, so every row rebuilds.
fresh: list[Summary] = [_pod("alpha"), _pod("beta"), _pod("gamma")]
table.show("pods", fresh, all_namespaces=False, pattern="")
Review of #209 pointed out that the clock test uses fresh summary objects, so it bypasses the memo entirely: removing the age string from the stamp would still leave it green while AGE cells froze forever for stable resources. Add `test_age_refreshes_for_an_unchanged_summary_as_time_passes`: it renders the *same* PodSummary object twice with the module clock frozen one minute apart and asserts the AGE cell moves 5m -> 6m. Verified as a real guard by mutation — forcing `_stamp()` to return None fails this test and the metrics test, and passes with the stamp intact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
hellices
added a commit
that referenced
this pull request
Aug 6, 2026
`test_node_shell_cleanup_failure_warns_and_audits` waited on the failure notification and then read the audit log after the app context closed. The outcome entry is appended *after* that notification, on a separate `asyncio.to_thread` hop, so a loaded full-suite run could tear the app down first and find only the `intent` entry. It now waits for the audit record itself. This originally also carried a `_rendered_cells` diff cache for `ResourceTable`; #209 landed the same optimisation on main as `_emitted`, with a memo and pruning on top, so that part is dropped in favour of the upstream implementation. Refs #186 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Aug 6, 2026
hellices
added a commit
that referenced
this pull request
Aug 7, 2026
* docs: design large-cluster performance qualification Define deterministic replay, guarded 1,000-Pod AKS load, evidence-gated optimization, cleanup, and publication requirements for #186. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: add versioned scale workload profiles Define strict deterministic profile inputs for issue #186. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs: clarify schema v1 initial state Schema v1 fixes the initial state to all Running/Ready Pods; future distributions need a new field.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: generate deterministic scale replay traffic Make object and event streams reproducible from the issue #186 profile seed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: expose optional Kubernetes read telemetry Measure logical API load for issue #186 without changing the unobserved runtime path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: reuse watch snapshot summaries Avoid duplicate generic watch summary projection, clean up reused item lists, and document the verified exception flow from review follow-up. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: report scale benchmark latency and resource use Add stable JSON and Markdown measurements for issue #186 comparisons. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: harden scale benchmark metric publishing Freeze published API aggregates, harden ProcessSampler lifecycle, and count relists only after 410 recovery. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: preserve tracemalloc across overlapping samplers Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: rollback process sampler startup failures Restore tracemalloc ownership and close the unstarted coroutine if startup fails after ownership acquisition. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: replay scale traffic through the real Textual app Exercise the production watch, store, render, and table path for issue #186. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: fix input-timing and oracle-pin defects in replay harness Two spec-compliance fixes for the Task 5 replay harness (issue #186): 1. Input timing: churn_start.set() was called after pilot.press(), so cursor input was measured while the replay source was blocked. Move churn_start.set() before the key presses and snapshot churn_started_before_input = churn_start.is_set() immediately before the first press. Both tests now assert report.churn_started_before_input. 2. Oracle independence: expected_digest was computed from _ReplaySource.current_digest() (internal parallel bookkeeping) rather than the spec-named apply_events() oracle. Switch run_replay to filter hard-failure sequences (gone/throttled/forbidden) from the event list and compute expected_digest = summary_digest(apply_events(...)). Both tests now assert report.expected_digest == summary_digest(apply_events(...)) as an independent oracle pin. For the gone-reconnect test the failure event (sequence 5) is correctly excluded from the oracle since it was never yielded as a watch event. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: add reproducible scale benchmark CLI Ship 1k, 10k, and 50k replay profiles with machine-readable reports for #186. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: harden benchmark CLI failures Narrow expected CLI failures, preserve unexpected tracebacks, stabilize JSON ordering, and cover dropped updates and argument validation for #186. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: fix absolute-offset delay bug in _ReplaySource; add time_scale=1 regression The watch-phase loop computed sleep durations from each event's absolute offset_seconds rather than elapsed wall-clock time, causing total sleep to equal the sum of all offsets (O(N × profile_duration)) instead of the profile duration. With smoke-1k (5 s, 180 events) the bug produced ~250 s of sleeping, guaranteeing a 30 s until() timeout on every time_scale=1 run. Fix: record self._replay_start when churn begins and compute delay = event.offset_seconds * time_scale - elapsed so each event waits only the time remaining to its scheduled position. Regression test: test_replay_time_scale_1_uses_relative_inter_event_delays uses a 3 s profile where the broken sum-of-offsets (~90 s) would exceed the until() guard. The test completes in 3.8 s on the fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: add digest-correctness assertion to time_scale=1 regression test Per code review: also assert report.expected_digest == report.final_digest so that a hypothetical event-reordering defect introduced alongside the timing fix would be caught even if dropped_updates stays zero. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: add time_scale=1 HTTP 410 reconnect regression; correct event-rate parameters Add test_replay_gone_reconnects_with_time_scale_1 which proves the relay harness correctly handles a HTTP 410 reconnect when time_scale=1 is active: the post-reconnect watch generation must continue using the original _replay_start (set at churn_start, not reset on reconnect) so event offsets remain relative to the global churn origin rather than re-sleeping the full absolute offset from the reconnect timestamp. RED/GREEN sensitivity confirmed: a temporary mutation that applied absolute- offset sleep for gen>0 (instead of offset*scale - elapsed) caused the churn to exceed the 30s until() timeout in the full test suite: AssertionError: churn complete and all events rendered not met within 30.0s Correct two adjacent test event-rate parameters: - test_replay_time_scale_1_uses_relative_inter_event_delays: 20 eps → 3 eps (sum-of-absolute-offsets with 20 eps = 90s, reliably showing the bug; 3 eps gives ~10s offsets, still > 30s with the bug, passes in 3.8s clean) - test_replay_gone_reconnects_and_digest_matches: 3 eps → 20 eps (reconnect test needs high event density to stress re-list correctness) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(task-7-rereview): restore test sensitivity and eliminate timing races Finding 1 (Critical): test_replay_time_scale_1_uses_relative_inter_event_delays was insensitive to the absolute-offset bug after steady_events_per_second was changed 20->3 in commit 2082aaf. With 9 events the sum_of_offsets is only 15 s < 30 s until() guard, so the test PASSES with the bug (false-GREEN confirmed: PASSED in 12.93 s with mutation). Fix: restore steady_events_per_second=20 (60 events, sum_of_offsets=91.5 s >> 30 s). RED confirmed: FAILED in 39.66 s with mutation. Finding 2 (Important): test_replay_gone_reconnects_with_time_scale_1 with duration_seconds=2 had only ~10.25 s margin above the 30 s until() guard, leaving ~1 s true safety when pilot.press() overhead is subtracted. Fix: increase duration_seconds=2->5 (95 post-failure events, sum_of_offsets=251.75 s, margin=221.75 s) for deterministic RED even in isolation. RED confirmed in isolation: FAILED in 39.67 s with reconnect mutation. Both tests now also use a sleep_callback (new ReplayOptions field) to accumulate total scheduled sleep time and assert total_sleep < duration_seconds * 3, providing a scale-independent deterministic check independent of the until() wall-clock guard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(task-7-follow-up): replace sleep_callback with virtual-time seam Replace the sleep_callback recording mechanism with an injected monotonic clock (monotonic_fn) and async sleeper (async_sleep) on ReplayOptions. _ReplaySource stores self._now and self._sleep, resolved at construction from the options; production runs use time.monotonic and _sleep_default (a thin asyncio.sleep wrapper). Tests supply virtual_monotonic / virtual_sleep closures that advance a shared virtual_time float and do asyncio.sleep(0) to yield without real wall time. Both time_scale=1 tests now complete in ~0.01 s instead of 3–5 s and their total_sleep assertions fire immediately (2.59 s total) under the historical absolute-offset mutation, with no dependence on the until() 30 s guard. RED evidence (mutation: delay = offset * time_scale, no elapsed subtraction): test_replay_time_scale_1_uses_relative_inter_event_delays: assert 88.5 < 9 FAIL test_replay_gone_reconnects_with_time_scale_1: assert 247.5 < 15 FAIL Both fail in 2.59 s wall time. GREEN: 4 passed in 3.97 s (all replay tests, correct implementation). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(task-7-review): pin replay timing across reconnects Require virtual sleep totals to match the final scheduled offset and assert the first post-410 delay. This makes the issue #186 regression fail for reconnect-origin resets and omitted sleeping, not only the original absolute-offset mutation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * feat: add live seed manifest generator Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(issue-186-task-8.2): add guarded real-AKS application-path replay Add tests/performance/live.py: run_live_replay drives the real production stack (KubeClient -> WatchManager -> ResourceStore -> MeasuredKorvidApp) against an already-seeded, uniquely-owned 20-namespace/1,000-pod AKS topology (built by manifests.build_seed_manifests). Fail-closed before any mutation: - Cluster identity gate: active kubeconfig context, resolved API-server hostname, and an independent `az aks show --ids` lookup must all agree. - Ownership gate: every expected namespace and pod must already carry both ownership labels; every mismatch is collected before raising. - Guarded churn: each mutation is a JSON-Patch that `test`s the target pod's UID and both ownership labels before `replace`-ing status.phase - a failed `test` op aborts the whole run, with no unguarded fallback. Reuses replay.py/metrics.py/workload.py/manifests.py/profile.py unchanged. Promoted namespace_name/pod_name/validate_run_id/MANAGED_BY_LABEL/ MANAGED_BY_VALUE/RUN_LABEL to public names in manifests.py so live.py shares the exact seeding contract instead of duplicating it. Extend cli.py with a `replay-live` subcommand (mandatory --profile/--context/ --expected-cluster-id/--run-id; optional --duration overriding only duration_seconds; no --time-scale - live churn always replays at real wall-clock time), reusing the same Markdown/JSON output and exit-code rules as `replay`. Add tests/performance/test_live.py (26 tests) and extend test_cli.py (+15 tests) covering: identity-gate rejections (wrong context/resource-id/ hostname/malformed JSON/nonzero exit/missing executable), topology mismatch, ownership-gate rejections, deterministic index-to-live-object mapping, guarded-patch construction, guarded-churn success/abort, namespace-filtered watch source, full happy-path digest parity, guard-failure and CancelledError propagation with client/watch teardown, and CLI argument/ error-handling coverage. Followed RED-GREEN-REFACTOR throughout; see the task report for RED evidence and exact verification results. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(issue-186-task-8.2-review): isolate harness reads from app-path API telemetry Address both Task 8.2 review findings in tests/performance/live.py: MEDIUM: run_live_replay wired a single KubeClient with recorder.record_api to both the ownership gate, the pre-churn uid snapshot, the app watch source, and the post-churn independent read, so ReplayReport.api mixed ~60 harness-only LISTs with the real application LIST/WATCH telemetry. LiveDependencies now exposes a second seam, harness_kube_client_factory, that constructs a non-instrumented KubeReadClient used only for _verify_ownership and the final independent re-read. The telemetry-wired client (kube_client_factory) is now only ever handed to make_live_watch_source, so ReplayReport.api reports exactly the production application read path. Identity ordering is preserved and tightened: the app-path client is now constructed only after the harness ownership gate passes (previously it was constructed and connected up front), and mutation_client_factory is still only called after ownership verification. LOW: _verify_ownership now returns the validated (namespace, name) -> PodSummary snapshot it already collected while checking ownership, and run_live_replay reuses that snapshot as the pre-churn uid snapshot instead of immediately re-listing every namespace's Pods a second time. Tests (TDD): _FakeKubeClient now models per-call telemetry on list_objects/list_pods (mirroring the real KubeClient) and tracks list_pods/list_objects/watch_pods call counts; _happy_deps constructs separate harness/app-path fake client instances sharing the same underlying namespaces/pods dicts. New/updated coverage proves: report.api.operations["list"] == 1 (only the app watch's own LIST, none of the harness's ~60 reads); the harness client is list_pods-ed exactly twice per namespace (ownership gate + final read, no redundant third uid pass); the app-path client's list_pods is never called; the harness client's watch_pods is never called; the application-path client is never even constructed when the ownership gate rejects; and _verify_ownership's return value matches the seeded topology. All prior Task 8.2 gate/churn/CLI tests are preserved and updated only where the LiveDependencies/_FakeKubeClient signatures changed. RED: reverting live.py to its pre-fix state with only the updated tests/performance/test_live.py in place produced "20 failed, 8 passed in 1.32s" (uv run pytest -p no:tach tests/performance/test_live.py -q), all failing on the new harness_kube_client_factory field/behavior. GREEN: uv run pytest -p no:tach tests/performance/test_live.py tests/performance/test_cli.py -q -> 55 passed; uv run pytest -p no:tach tests/performance/ -q -> 102 passed; uv run ruff check/format, uv run mypy tests/performance/live.py tests/performance/cli.py, and uv run tach check all pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(issue-186-whole-branch-review): make the live qualification harness trustworthy Independent whole-branch review of the #186 performance harness found three blocking defects, seven important gaps, and fifteen quality findings. This closes every credible one. Blockers - Record event-to-render at owned MODIFIED *watch receipt* instead of after the patch acknowledgement. The watch event and the patch response race over independent connections, so recording at ack could append a pending entry after its own render (reproduced: a 60s opaque wait timeout and false dropped updates) and silently folded the write round trip into a read-path latency metric that is compared against the 1k/10k/50k baselines. - Churn a dedicated non-ownership `korvid.dev/performance-tick` label on the Pod's own metadata instead of the kubelet-owned `status.phase`, keeping the atomic UID and both ownership-label JSON Patch `test` operations. Read the ground-truth cluster snapshot while the watch is still live, revalidate ownership, wait for the store digest to converge, and re-assert the exact row count before teardown. - Drive mutations with explicit bounded concurrency and a bounded per-attempt timeout, retry only HTTP 429 with a bounded policy re-issuing the identical guarded patch, and report requested events/rate next to observed events, churn wall time, achieved rate, and mutation throttles (counted separately from application read telemetry). Important - Add `aks-live-1k` (1,800s at 20 events/s with three 30s bursts at 100 events/s), matching the published live plan, and point the command help and design doc at it; `aks-1k` stays the deterministic comparison schedule. - Revalidate profile invariants after `--duration` in both the CLI and `run_live_replay`, with an explicit error before any identity/ownership work. - Cancel and drain the churn task (and every mutation task inside its task group) before any client is closed, proven by a real outer-task cancellation test. - Reject unexpected Pods in owned namespaces, and reject any expected Pod that lost its UID identity or either ownership label on the post-churn read. - Make an injected `forbidden` replay failure a named terminal abort instead of a 30-second render timeout; add direct throttled/forbidden/slow coverage. - Name the recorded API errors in wait/convergence timeouts. Quality - Remove dead `current_digest`; add `BenchmarkRecorder.pending_count()`/ `api_errors()`; replace the tautological churn/input ordering flag with a real emitted/dispatched-event signal; give `until` a dedicated `WaitTimeout`; add `get_object` error telemetry; reuse the bound LIST payload items; use the injected clock for live input latency; handle output-path `OSError`; untrack the `.superpowers/sdd/task-4-report.md` session artifact; promote `build_manifest`; connect the mutation client eagerly under a bounded timeout; carry `object_index` on `ScheduledEvent` instead of parsing Pod names; bound the overall churn wait; count only resource-update renders. - Keep exact decoded-byte accounting: measured at 8.8ms for a 1,000-Pod LIST (0.4% of the 2s budget) and 9us per watch event, while the suggested `len(str(payload))` saves 1.3ms and reports Python repr characters instead of bytes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(issue-186-review): honor mutation Retry-After hints Preserve Kubernetes 429 retry metadata, bound retry delays, and spread concurrent workers with deterministic target-specific jitter.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(issue-186-review): desynchronize hinted retries Treat Retry-After as a server floor and add stable target-specific jitter before applying the configured ceiling, preventing workers from retrying in lockstep when the hint dominates exponential backoff.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(issue-186-review): report terminal replay aborts Handle expected terminal replay failures as clean CLI errors while preserving propagation of unexpected programmer defects. Add a valid forbidden-failure profile regression test that proves no traceback is emitted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(issue-186-review): complete PR #202 qualification review wave Address all inline review findings for the large-cluster qualification harness: - generated Pods tolerate the documented korvid.dev/performance taint - fail-closed AKS gate validates the immutable dedicated-test resource group, cluster name, and required test-only tags from az aks show before clients - ownership/preflight rejects non-Running/non-Ready owned Pods (exactly 1000) - explicit read/setup connection timeout on identity, harness, and app connects - resolve the real korvid SHA (GITHUB_SHA/git HEAD); live fails closed if none - live-specific manifest records context/ARM id, Kubernetes and node-pool version/count metadata, persisted to JSON and Markdown - separate LIST-to-populated-table/startup phases from watch event-to-render - machine-readable process-start-to-interactive, LIST-to-populated-table, max backlog depth, and post-burst drain summaries - drive filter, sort, namespace switch, split pane, describe, and multi-log UI-at-scale scenarios through the real pilot during churn, recording outcomes - extend failure vocabulary with metrics_unavailable and slow_logs plus report evidence - require exactly four run-labelled live artifacts for replay-live - persist expected/final digest and an explicit match flag - fit the RSS slope only over post-warm-up steady-state samples - correct the design namespace contract to match the generator The audit-injection finding is rejected: the audit invariant governs product agent write tools, not this dedicated performance harness; the stronger cluster/ownership/UID JSON-Patch guards are preserved and tested. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(issue-186-review): bound az aks show call in live identity gate The fail-closed live cluster identity gate awaited the first external `az aks show` command without a timeout, so a stuck kubeconfig exec / credential plugin could hang the gate indefinitely before any client was constructed. Wrap the command_runner await in `asyncio.timeout(limits.read_connect_timeout_seconds)`, consistent with the adjacent context-host lookup and setup connects. TDD: added test_run_live_replay_bounds_the_az_aks_show_lookup mirroring the context-host bounding test (RED: hung ~31s and raised ValueError; GREEN: bounded, raises TimeoutError in <0.05s). Full tests/performance/test_live.py: 72 passed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: use supported az aks show args Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(k8s): refresh expiring exec credentials across configuration copies Long-running sessions against AKS died with HTTP 401 after ~22 minutes: kubelogin's exec credential expired and nothing re-ran the exec plugin. `load_refreshable_kube_config()` now installs a `refresh_api_key_hook` that re-runs the kubeconfig loader shortly before the exec credential expires. Because `Configuration.set_default()` and a default-constructed `ApiClient()` both deep-copy the configuration, a shared refresh closure alone is not enough - each copy owns its own `api_key`. The hook therefore tracks a shared refresh generation and re-applies the loader's cached credential to any copy that has fallen behind, so every clone converges without re-spawning the exec plugin. Refresh is serialized behind a lock, bounded by the existing probe timeout, and errors propagate instead of being swallowed. A failed refresh deliberately leaves the generation untouched so other copies still attempt a refresh rather than trusting a stale token. `connect()`, `switch_context()`, `open_pod_exec()` and the live performance mutation client now pass the active configuration explicitly to `ApiClient`/`WsApiClient` so refreshed credentials actually reach the transport. Tests: tests/k8s/test_client.py (refresh, serialization, stale-copy propagation, static-token no-op, generation-invariant on failure, connect/switch/exec wiring) and tests/performance/test_live.py::test_mutation_client_connect_uses_refreshable_kube_config Refs #186 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(ui): wait for the node-shell audit outcome, not the notification `test_node_shell_cleanup_failure_warns_and_audits` waited on the failure notification and then read the audit log after the app context closed. The outcome entry is appended *after* that notification, on a separate `asyncio.to_thread` hop, so a loaded full-suite run could tear the app down first and find only the `intent` entry. It now waits for the audit record itself. This originally also carried a `_rendered_cells` diff cache for `ResourceTable`; #209 landed the same optimisation on main as `_emitted`, with a memo and pruning on top, so that part is dropped in favour of the upstream implementation. Refs #186 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(perf-cli): fail live runs on failed UI scenarios and aliased artifacts Two review findings on the live qualification command: `drive_ui_scenarios` deliberately records a key sequence that never reached its target state as `ScenarioResult(ok=False)` instead of raising, so the scenario outcomes never reached the exit status. A live run whose filter, split-pane, describe, or multi-log evidence failed still reported success — exactly the case where the qualification has nothing to show. `replay-live` now folds failed scenarios into its exit status and names them on stderr. Artifact distinctness compared raw path strings, so aliases such as `sub/../run.json` and `run.json` passed the check while resolving to the same file, letting a later artifact write silently destroy an earlier one. Distinctness is now decided on resolved paths. The design doc records both: failed UI-at-scale scenarios join the hard budget table at 0. Tests: tests/performance/test_cli.py:: test_cli_replay_live_fails_when_a_ui_scenario_did_not_pass test_cli_replay_live_rejects_artifact_paths_that_alias_one_file Refs #186 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(perf-live): make UI-at-scale scenarios prove what they claim Review round on PR #202 found the UI-at-scale evidence was hollow: a scenario was marked `ok=True` whenever `pilot.press` returned, without checking any resulting state. In the live wiring that meant three of the six scenarios could not have done anything and still "passed": - `describe` and `multi_log` bail out with an "unavailable" warning because no manifest/log provider was wired; - the namespace toggle was a no-op, because the configured namespace was already `ALL_NAMESPACES`, so `0` navigated to the scope it was on. None of those raises. Combined with the previous commit folding scenario outcomes into the exit status, a run could have "passed" on no evidence. Each scenario is now an ordered list of steps that each declare the observable state they must reach (filter pattern, active sort, current scope, pane count, describe screen, log pane) and wait for it; a step that does not get there records `ok=False`. The remaining steps still run so the workspace is restored and the digest/row-count convergence checks stay intact. To make the scenarios reachable the live app now gets read-only providers on the harness connection - a pods-only `get_manifest` and the read client's `stream_logs` - plus a favorite namespace so `1`/`0` really scopes down to a seeded namespace and back. That scope change restarts the application watch, which the happy-path test now pins explicitly: three watches, all still cluster-wide, because `make_live_watch_source` keeps the read pinned to the owned namespace set regardless of UI scope. Two further findings from the same round: - The live churn driver never marked burst boundaries, so every live report left `post_burst_drain_seconds` empty and printed "n/a" for the published <=3-second burst-drain budget. It now marks each boundary on the same clock the deterministic driver uses. - `load_profile` accepted duplicate `at_event` positions, which `run_replay` silently collapses into one injection while the profile hash and report still claim both. Duplicates are now rejected at load. Also extends the earlier node-shell audit-race fix: four more tests waited on a notification and then read an audit outcome that is appended afterwards on a separate `asyncio.to_thread` hop. They now share one helper that waits for the outcome entry itself. Tests: tests/performance/test_live.py:: test_ui_scenarios_are_not_marked_ok_when_the_app_state_never_changes test_run_live_replay_times_the_post_burst_drain tests/performance/test_profile.py:: test_load_profile_rejects_duplicate_failure_event_positions Refs #186 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(perf-live): verify the rendered table and stop miscounting reconnects Second review round on PR #202. Three of these are defects the previous round's changes created or exposed. **The digest criterion never checked the table.** `expected_digest` and `final_digest` are both store digests, so a table showing 1,000 stale rows satisfied the published store/table digest criterion — and the row count was the only thing asserted about the rendering. That is exactly the guarantee the new cached in-place diff needs, since it now diffs against its own record of what it last wrote instead of reading the table back. `check_rendered_rows` projects each owned Pod onto the strings its row must display and checks them against the cells the `DataTable` actually holds. It is written from the Pod summary rather than by calling the widget's row builder on purpose: reusing the builder would only prove the widget agrees with itself. **Reconnects counted deliberate restarts.** `reconnects` was inferred as `watch_open - 1` per path. The `namespace_switch` scenario added last commit legitimately re-opens `/api/v1/pods` twice, so a perfectly healthy run reported two reconnects. A reconnect is now counted from recovery: a `watch_open` on a path whose stream previously errored. **An empty backlog at a burst boundary produced a bogus drain.** `mark_burst_end` always queued a pending marker, so with nothing to drain the next unrelated steady-state render reported its own latency as the drain (or the sample vanished if no later render arrived). Nothing to drain now records `0.0` immediately. **`_flush_allocation_snapshot` could mask a run failure.** It runs from a `finally`, so an unwritable destination raised `OSError` past the command's handler: a traceback after a 30-minute run, hiding whatever actually failed. It now reports and returns a status, tracing is always stopped, and the failure surfaces as exit 1. Docs: the published live command was no longer executable (all four artifacts are mandatory and each filename must carry the run id), and the determinism claim listed uids and resource versions, which the generator does not assign — on a live run they are whatever the cluster issued. Tests: tests/performance/test_live.py::test_rendered_rows_check_rejects_a_stale_cell tests/performance/test_metrics.py:: test_reconnects_count_only_watch_reopens_after_an_error test_reconnects_count_a_watch_reopened_after_a_dropped_stream test_mark_burst_end_records_zero_when_the_backlog_is_already_empty tests/performance/test_cli.py:: test_cli_replay_live_reports_an_unwritable_allocation_snapshot Refs #186 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(perf): stop double-marking bursts and leaking tracing on sampler failure Third review round on PR #202 raised no blocking comments; these are the suppressed findings that turned out to be real correctness bugs, two of them in the burst-drain machinery the previous commits touched. **A reconnect re-marked every burst that had already ended.** `_ReplaySource` restarts at `_next_event_index` on a new watch generation, but the burst cursor was a local reset to zero each time. After a 410 drops the stream mid-schedule, the first resumed event re-marked every closed burst. Reproduced with a single-burst profile and a 410 at event 50: two drain samples for one burst, the second a spurious `0.0`. The cursor now lives on the source, alongside `_next_event_index`. **A failed sampler leaked managed tracing.** `ProcessSampler.stop()` awaits the sampler task and released tracemalloc afterwards, so a `psutil`/`tracemalloc` error inside the task raised past the release - and past the caller's own watch-manager teardown. The release now runs from a `finally`. **The offline replay never checked its rendering either.** Both digests are computed from the store, so a table full of stale cells passed - the same hole `28dec94` closed on the live path, and the one that matters for the cached in-place diff. `check_rendered_rows` moved from `live.py` to `replay.py` (`live.py` already imports from it, so this avoids an import cycle) and now runs on the offline path too, inside the app block where the table still exists, and only when the run was not aborted by an injected failure. Tests: tests/performance/test_replay.py:: test_replay_does_not_re_mark_bursts_after_a_watch_reconnect tests/performance/test_metrics.py:: test_sampler_stop_releases_tracing_even_when_the_task_failed Refs #186 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs: publish the measured 1,000-pod scale envelope The qualification design says main carries compact baseline and optimized summaries plus the supported scale envelope and known limits. It did not. This adds them from the recorded live runs rather than leaving the numbers only in the issue thread. Records both budgets that pass and the two that miss: event-to-render p95 (299ms against a 250ms budget, but measured at 24 ev/s against a budget written at 20) and cursor-input p95 (2.4s against 100ms, more than 20x over and untouched by the render work). Also flags what is not trustworthy yet: the UI-at-scale interaction timings predate the harness fix that makes those scenarios wait for the target state, so they are upper bounds taken under CPU saturation, not measurements. Refs #186 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * chore: retrigger CI after Actions outage The CI run for ade1fd0 has been stuck in queued for ~12 hours since the GitHub Actions major outage on 2026-08-06; its jobs API returns zero jobs and it can neither be cancelled nor re-run. Pushing an empty commit to get a clean run now that hosted runner queues have drained. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Fixes #208
Problem
Every watch event triggered a full-table repaint whose cost was proportional to
the total row count, not to the number of objects that changed — and it ran
synchronously on the asyncio event loop, so keypresses queued behind it.
Measured before this change (pods view, all-namespaces, M-series Mac, 3.12):
At 10,000 objects, 10 events/second was already enough to push cursor movement
past 2.5 seconds.
cProfileat 50,000 pods showed where it went:rich.Text.__init__— 400,000 allocations per repaint (~8 per row), ~52% ofrender time, each running
strip_control_codes/str.translateformat_age— adatetime.now(UTC)and an RFC 3339 reparse per row, perrepaint
_apply_in_place— oneDataTable.get_row()per row, each re-derivingordered_columns(50,002 calls), then a 14-cell value comparison — ~300 mseven when nothing had changed
Approach
Watch summaries are frozen dataclasses replaced wholesale by
apply_event, soa row whose summary object is unchanged provably renders identical cells. That
makes object identity a sound cache key.
ResourceTable._row_memokeyed by summary identity plus astampcovering the inputs not carried by the summary — the age string, and the
live metrics sample on the pods view. A hit re-emits the memoised row
untouched. Invalidated when the cell-set shape changes (kind,
all-namespaces, custom view); not on sort, which reorders rows and
decorates headers without changing any cell. Pruned amortised so churning
pod names cannot grow it without bound.
_emitted— the cells this widgetlast put into the table — instead of
DataTable.get_row(). A memo hit isthe same list object, so it is settled by one identity check and never
touches
ordered_columns,get_row, or_cells_equal.get_rowremainsthe fallback whenever
_emittedhas no record of a row, so the diff stayscorrect even if the bookkeeping ever drifted.
datetime.now(UTC)per repaint, threaded into everyage()call.Also fixes a latent inconsistency: rows in a single repaint could previously
straddle a second boundary and report different ages for the same instant.
Results
Same fixtures, after:
Keypress latency under churn (30 samples, cursor-down, 10 events/s):
Cold first build is unchanged by design (there is no memo to hit) and is now
the dominant cost at 50,000 rows — a separate bootstrap problem, not folded in
here.
Tests
tests/ui/test_table_row_memo.py, all behavioural, no wall-clock assertions:test_repaint_rebuilds_only_the_changed_row— one MODIFIED pod rebuildsexactly one row
test_unchanged_repaint_rebuilds_nothing— an identical repaint rebuilds nonetest_unchanged_rows_are_not_read_back_from_the_datatable— the diff makeszero
get_rowlookups (Textual's own paint-timeRowKeylookups filtered out)test_changed_metrics_rebuild_the_row_without_a_new_summary— guards againstover-caching: a new metrics sample re-renders the row even though the summary
object is identical
test_view_signature_change_discards_the_memo— switching to theall-namespaces column set rebuilds every row
test_age_uses_one_clock_reading_per_repaint— a singlenowfeeds every rowThe first three and the last failed RED against
main; the two guard tests passeither way by design (they exist to catch over-caching).
Verification
make check— ruff, mypy --strict, 4072 passed / 21 skipped, tach all green