Skip to content

fix(runtime-host): serve revision-consistent Usage snapshots - #4068

Open
Sun-GLiang wants to merge 23 commits into
apache:mainfrom
Sun-GLiang:fix/4058-usage-snapshot-consistency
Open

fix(runtime-host): serve revision-consistent Usage snapshots#4068
Sun-GLiang wants to merge 23 commits into
apache:mainfrom
Sun-GLiang:fix/4058-usage-snapshot-consistency

Conversation

@Sun-GLiang

@Sun-GLiang Sun-GLiang commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • keep one repaired SQLite Usage/Pricing view revision-pinned across every page while preserving the existing bounded page and retry contracts
  • lease each snapshot to its initiating IPC connection, reserve one of four slots before expensive capture, cap one connection at two slots, and retry transient capacity conflicts as bounded whole Desktop loads
  • serialize Usage settings reloads per Host generation, collapse superseded queued range changes to the latest request, and keep distinct Host connections independent
  • reclaim leases on explicit release or disconnect, renew a five-minute idle lifetime on owner access, and retain a 30-minute hard lifetime
  • release Desktop leases from a finally path, preserve the latest main session-title hydration with bounded concurrency, and raise the merged Runtime Host compatibility epoch to 118
  • cover overlapping starts, rapid range changes, A → B → A Host reuse, idle and hard expiry, wrong-owner access, disconnect cleanup, failed Desktop loads, and protocol correlation

Fixes #4058

Verification

  • full Desktop suite on 9f9dd0a20: 2,240 passed
  • focused Usage feature scope suite: 9 passed
  • Desktop typecheck
  • Desktop main build
  • targeted Biome check
  • git diff --check
  • full Runtime Host suite on the prior reviewed head: 1,724 passed; 12 skipped (Runtime Host code unchanged by the latest remediation)
  • full Storage suite on the prior reviewed head: 1,110 passed; 8 skipped (Storage code unchanged by the latest remediation)
  • protocol epoch guard: 117 → 118

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex implemented and tested the review remediation, Desktop/Runtime Host lease lifecycle, capacity reservation, and merge-conflict resolution. Sun-GLiang is the human contributor of record.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/XL Under 2500 readable lines label Aug 28, 2026
…shot-consistency

# Conflicts:
#	packages/runtime-host/src/protocol/index.ts

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for moving Usage reads onto one coherent revision; the Storage/Host/Desktop ownership is much clearer now. I found one remaining lifecycle boundary where a valid reader can lose its revision mid-pagination. This is a suggestion from an outside review, so please feel free to push back if the supported concurrency or latency envelope is intentionally narrower.

AI-assisted review disclosure: Codex ran independent authority and production/test analysis lanes; Astro-Han is the contributor of record for this review.

Comment thread packages/runtime-host/src/server/usage-snapshot-cache.ts Outdated
…shot-consistency

# Conflicts:
#	apps/desktop/src/main/runtime-host-client.ts
#	packages/runtime-host/src/protocol/index.ts
…shot-consistency

# Conflicts:
#	docs/windows-test-inventory.md
#	packages/runtime-host/src/protocol/index.ts
#	packages/runtime-host/src/server/operation-dispatcher.ts

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lease rework closes last round's point properly: reserve() has no eviction path, capacity is taken before capture (the barrier test proves the fifth start is refused before any title read), idle renews under a hard cap, ownership is checked on read and release, and release rides the existing releaseConnection seam with a real five-client UDS test for disconnect. Capture is one BEGIN IMMEDIATE on the shared handle, so the consistency claim holds in-process, and paging no longer re-runs the unbounded model-call read per page. Build, 55 host and storage tests, 15 desktop tests, lint, format, typecheck and the architecture check are green locally on bdf0faf5; merge-tree against main is clean.

One thing to fix before merge. Your reply says the fifth start "returns the typed revision_changed result"; the code returns operation_conflict, and loadUsageSnapshot only retries on revision_changed, so the error reaches the Usage page as a hard failure. Two supported paths get there: a single Desktop connection switching ranges quickly holds all four slots until its own finally runs, so a second window or another client fails outright; and a best-effort release that times out on a remote Host leaves the lease until idle expiry, so four of those is five minutes of failure for everyone. Both are recoverable, but #4058 item four asks for a bounded retry of the whole load. Smallest fix: a per-connection cap in reserve() (one is enough for Desktop) and operation_conflict inside the existing MAX_USAGE_SNAPSHOT_ATTEMPTS loop with a short backoff.

Things this PR makes redundant and should take with it: after loadAllLogs goes, usage.query kind: 'logs' has no production consumer, and the Desktop usage:logs and usage:buckets handlers were never exposed by preload on main either. Deleting those two handlers, loadAllBuckets, the logs/buckets protocol variants and the coordinator's usageLogPage family, then folding the old and new page builders, is a few hundred lines of net deletion on an epoch this PR already bumps. Keep kind: 'summary', Session Inspector uses it.

Smaller: the 50,000 activity cap lives in both usage-snapshot-cache.ts and runtime-host-client.ts, and the Desktop copy turns a legitimately larger Host page into invalidProjection; carry it in snapshot_started or the protocol. retain() on the cache is test-only, production goes reserve then finalize. The inner transaction('read') inside the outer write transaction is a pass-through at depth one, and the third acquireOperationalStateDatabase can come from the repos' existing lease. The started.kind !== 'snapshot_started' branch sits outside the try and is unreachable after assertUsageQueryOutputForInput, and the test that pins it can go with it. The body still says epoch 79; the code is 95, which #4386, #4308, #4439, #4500 and #4508 also claim, so re-check at merge.

Evidence boundary: static read of bdf0faf5 against main 92fa5281; runtime-host, storage and desktop usage suites run locally; Playwright not run, and the two-window and rapid-range scenarios are traced, not exercised.

AI-assisted review: drafted with Maka; I verified the capacity error path, the Desktop retry condition and the preload exposure myself.

简体中文

租约改造把上轮的点关干净了:不驱逐、先占容量再捕获、idle 续期加硬上限、归属校验、断连回收走现有接缝,一次 BEGIN IMMEDIATE 保证进程内一致性。本地验证全绿。合并前要修一处:第 5 个 start 实际返回 operation_conflict 而非你回复里说的 revision_changed,Desktop 只对 revision_changed 重试,所以用户看到的是硬失败;单连接快速切 range 就能占满四个 slot 饿死其他客户端。最小修法:reserve() 加每连接上限,并把 operation_conflict 纳入现有重试循环。本 PR 让 usage.query 的 logs 变体和 Desktop 两个从未经 preload 暴露的 IPC handler 变成死代码,建议同 PR 删掉。其余为小项:50,000 上限双权威、test-only 的 retain()、无效的事务嵌套、不可达的 kind 分支、正文 epoch 79 应为 95。

Comment thread packages/runtime-host/src/server/usage-pricing-coordinator.ts
Comment thread apps/desktop/src/main/runtime-host-client.ts
…shot-consistency

# Conflicts:
#	packages/runtime-host/src/protocol/index.ts
Resolve the Runtime Host protocol epoch conflict by placing the Usage snapshot wire contract at epoch 100 after main's epochs 97-99. Fence the WorkHub layout E2E first send on the shared send-readiness signal; an unfenced Enter could be silently dropped while submission admission was still initializing.

Generated-by: Codex
The prior run stopped when Node 24 could not deserialize its test-runner child payload. The affected release-contract file is unchanged and passes 10/10 isolated repetitions locally.\n\nGenerated-by: Codex
Bound snapshot leases per connection while preserving one replacement load, retry capacity conflicts as whole Desktop loads, and enforce the shared activity ceiling at the protocol boundary.\n\nGenerated-by: Codex
@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

Review disposition for 3415280:

Implemented:

  • retry usage.query / operation_conflict as a bounded whole Desktop load (3 attempts, 50 ms delay);
  • add per-connection fairness on top of the global four-slot cap;
  • share and enforce the 50,000-row ceiling at both Host configuration and protocol decoding;
  • remove the test-only retain() shortcut;
  • flatten the redundant nested read transaction;
  • update the compatibility test and PR text to epoch 100.

I did not apply the remaining suggestions verbatim:

  • Per-connection capacity is 2, not 1. One current load plus one replacement/range-switch load is a normal same-connection overlap; a cap of 1 would turn it into self-contention. A cap of 2 still prevents one connection from consuming all four global slots.
  • The legacy Usage variants cannot be deleted as dead code in this PR. Production main-process code still calls filtered kind: "logs" in latestRuntimeProbe() (runtime-host-permissions-ipc-main.ts:130), and the Usage IPC main module still issues summary, logs, and buckets queries. Removing them would be a separate API migration, not cleanup local to fix(runtime-host): serve one revision-consistent Usage snapshot #4058.
  • The extra acquireOperationalStateDatabase(root) is the shared, reference-counted transaction seam used to pin the capture. Reaching through repositories for their internal leases would widen and couple those abstractions.
  • The post-decode snapshot_started guard remains as defense in depth and keeps the client safe for alternate/mock RuntimeHostConnection implementations.

Fresh local verification: full build; Runtime Host 1,618 passed / 12 skipped; Desktop 1,983 passed; Storage 1,086 passed / 8 skipped; typecheck, lint, format, protocol epoch guard, and independent review all passed.

Resolve the concurrent Runtime Host wire changes at compatibility epoch 101.\n\nGenerated-by: Codex
@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

Follow-up after the latest main sync: main advanced while the review fixes were being pushed and introduced a separate wire change at epoch 100. The merge conflict is resolved in f599d71 by preserving that history and moving this PR's Usage snapshot wire change to epoch 101; the predecessor-handshake test now rejects epoch-100 peers. CI run 33719193127 passed in full: https://github.com/apache/maka/actions/runs/33719193127

Preserve the revision-pinned Usage snapshot contract above main's protocol epoch 105 and update snapshot fixtures for the required recorded-duration summary field.\n\nGenerated-by: Codex
Preserve main's compatibility epochs 106 and 107, and move the revision-pinned Usage snapshot wire contract to epoch 108.\n\nGenerated-by: Codex
@github-actions github-actions Bot added effort/XXL Over 2500 readable lines and removed effort/XL Under 2500 readable lines labels Sep 3, 2026
Preserve main's form-interaction compatibility epochs 108 and 109, and move the revision-pinned Usage snapshot wire contract to epoch 110.\n\nGenerated-by: Codex
…shot-consistency

# Conflicts:
#	apps/desktop/src/main/runtime-host-client.ts
#	packages/runtime-host/src/protocol/index.ts
Preserve main's compatibility epochs 111 and 112, and move the revision-pinned Usage snapshot wire contract to epoch 113.\n\nGenerated-by: Codex

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head 4b818bc03d7ba4770e14d1ef991af8f7c3da515b (OPEN). Code GO — no P0–P2, three P3 observations below. This is feature-grade (protocol change + behavior change + epoch bump): no approve from me, merge decision belongs to humans, and the epoch-113 ordering must be handled at merge.

P3s (non-blocking)

  1. The PR body still says epoch "101" — stale (actual 113 = main 112 + 1; code and commit message are correct).
  2. Epoch-113 merge-order dependency: #4068 shares 113 with #3299 and #4713 — whoever merges first takes the slot, the rest must rebase.
  3. If capture exceeds the 5-minute idle window, finalize returns undefined → "reservation no longer active" → bounded client retry; extremely large repos could fail repeatedly (repair is bounded 16×512 with a 50k cap — real risk low, recorded).

Scope and limits

All 19 production files plus key test files (cache/coordinator/protocol/desktop client+ipc/storage) and both prior review rounds read in full. Tests not run locally (CI test fully green bound to this head plus the author's verification matrix covering 3 suites; full runs belong to CI on this machine class). Real dual-window / fast-range-switch scenarios traced, not exercised.


Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

简体中文

评审结论来自自动化审查流程;发布者没有读这份 diff,核的是当前 head 有没有漂移。当前 head 是 4b818bc,未关闭。代码无阻断问题,三条小的都是纪元与边界记录。 rifle feature 级改动,合并由人类定,注意纪元顺序。

@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

Disposition for review 5122230496:

  • Updated the PR body to match the reviewed head: compatibility epoch 113 and protocol guard 112 -> 113.
  • No code change for the merge-order observation. The current main branch has since advanced to epoch 117 and this PR is now behind/conflicting, so the final epoch must be reassigned to latest main + 1 when the branch is rebased for merge. Open protocol-changing PRs can make any number chosen earlier stale again.
  • No code change for captures exceeding the five-minute idle window. This remains a bounded, low-probability residual risk: the 50,000-row ceiling and bounded repair limit capture work, expiration releases the reservation, and Desktop retries the whole load a bounded number of times. If production telemetry shows captures approaching the idle deadline, the follow-up should distinguish active pending capture time from retained-lease idle time rather than simply widening the timeout.

@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

Updated the branch against main at 411512bd.

  • Resolved the protocol epoch conflict by preserving main's epoch 113–117 history and moving this PR's Usage snapshot wire change to epoch 118.
  • Updated predecessor handshake coverage to reject epoch-117 peers.
  • Preserved the PR's 19-file scope relative to current main.

Post-merge verification:

  • npm run build:test
  • npm run typecheck
  • Storage: 1,110 passed / 8 skipped
  • Runtime Host: 1,724 passed / 12 skipped
  • Desktop: 2,222 passed
  • npm run format:check
  • npm run lint
  • protocol epoch guard: 117 → 118
  • protocol epoch guard tests: 13 passed
  • git diff --check

The Website suite is not reported as passing locally because the shared worktree node_modules has an existing Astro/cookie CommonJS/ESM mismatch noted earlier; no Website files are part of this PR diff.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up to my review above, new head e59cf432345db20efafdfe797268acfdae7c4c55 (OPEN, MERGEABLE/BLOCKED awaiting human review). Code GO — no P0–P2, the three P3s stand. Verified for this head rather than carried over: the PR's own 19 production/test files are byte-identical between 4b818bc0 and this head (only the merged main moved); the epoch is correct (PR 118 = main 117 + 1, bumped continuously, no regression, epoch guard passes); CI test bound to this exact commit is success. P3s unchanged: ① body still says epoch "101" (actual 118); ② the 118 ordering dependency stays (same pool as later PRs now, tracked separately); ③ the theoretical capture-past-5min-idle-window finalize risk stands. Feature-grade: no approve from me, merge belongs to humans (no APPROVED bound to this head yet).


Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

简体中文

评审结论来自自动化审查流程;发布者没有读这份 diff,核的是当前 head 有没有漂移、以及 exact-head 的门禁状态。当前 head 是 e59cf43,未关闭。代码逐字节没变,纪元正确,检查绿,三条 P3 维持。功能改动,合并由人类定。

@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

Correction and disposition for follow-up review 5123759075:

  1. The stale-body observation no longer applies. The current PR body contains no epoch 101; Summary says epoch 118 and Verification says 117 -> 118. This was re-verified through the GitHub API after the follow-up review was posted.
  2. The epoch ordering dependency is a merge-time invariant for every open protocol-changing PR, not a defect remaining in this branch. Current main is still epoch 117, this head is epoch 118, the epoch guard passes, and GitHub reports the PR as mergeable. If another protocol PR takes epoch 118 first, this branch must be advanced again immediately before merge.
  3. The capture-past-five-minute case remains an explicitly recorded, low-probability residual risk rather than an unaddressed requested fix. Capture work is bounded by the 50,000-row ceiling and bounded repair, expiration releases the reservation, and Desktop performs bounded whole-load retries. A follow-up fix should separate pending-capture lifetime from retained-snapshot idle lifetime instead of merely widening the timeout.

CI test for exact head e59cf4323 has completed successfully. The review has no inline threads to resolve.

Keep main's test-tier migration by retiring the WorkHub geometry E2E after its assertion moved to Storybook.

Generated-by: Codex

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at 8bf3ddf (a merge of main into the branch with no change of your own over e59cf43) against main (a5a99a633), whole PR, clean merge; main is at epoch 117 and the head declares 118, which is right.

Your corrections in the last comment: the body epoch is indeed 118 now, and the ordering note stands. The design holds: #4058 is a contract defect readable straight from main's loadUsageStats (three independent reads under Promise.all), and the capture now happens once inside one transaction('write') on the shared acquireOperationalStateDatabase handle (usage-stores.ts:511), with the lease released through the existing releaseConnection seam. Everything from the earlier rounds is closed except one, and one of the corrections is wrong.

P2 (path ①): switching the range three times in quick succession fails the page. usage-settings-view.tsx:91 reloads on every range change, services-context.tsx:112 is last-write-wins and never releases the superseded load, and the selector has no guard. So three clicks are three concurrent snapshot_start on one connection; the per-connection capacity is 2 (usage-snapshot-cache.ts:36), the third gets operation_conflict, and the Desktop's retry is three attempts 50ms apart (runtime-host-client.ts:169, :1402-1421), about 100ms against a capture that takes longer than that on a real store. The rejected load is the newest ticket, so the user gets the usage_unstable toast and the page stays on the previous range. On main the same clicks succeed. Tightening the cap from a global 4 to 2 per connection in the last round made this reachable in three clicks instead of five. Smallest fix: at most one in-flight snapshot load per connection in UsageFeatureScope.reload (or in Desktop main), releasing the superseded reservation before starting the next; the renderer already discards the old result, so cancelling loses nothing.

P3:

  • Correction 3 in your comment ("Desktop performs bounded whole-load retries" for a capture that outlives the idle window) does not hold. reserve() fixes idleExpiresAt at reservation time and capture does not extend it (usage-snapshot-cache.ts:129-139); after expiry finalize returns undefined, #startUsageSnapshot throws a plain Error (usage-pricing-coordinator.ts:355), #mapReadFailure lands in unknown and rethrows, and the Desktop retry loop only covers operation_conflict. Path ④ on today's stores, so P3, but the sentence in the body should go.
  • usage:logs / usage:buckets IPC handlers, loadAllBuckets and kind: 'buckets' are not exposed by the preload on main and have no other caller; kind: 'logs' is still used by latestRuntimeProbe. Pre-existing, not for this PR, but "legacy variants cannot be deleted" is only true of logs.

Manual check before merge: the Usage page with a 50k-row store, switch range rapidly five times, then leave it idle six minutes and switch once.

Evidence boundary: static read; the three-click race and the idle expiry are traced through the code, not reproduced.

AI-assisted review: drafted with Maka; I verified the capacity constant, the retry budget, the non-cancelling reload and the toast path myself.

@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

Disposition for review 5124157985:

  • Fixed the P2 rapid-range race in 9f9dd0a20. UsageFeatureScope.reload now serializes loads by Host-generation key, skips superseded queued ranges before they reach loadUsageStats, and starts the latest queued load only after the active load has returned through its release path.
  • Retained unfinished lanes by key, so A → B → A cannot open a second snapshot on the still-active A connection; B remains independent and may load concurrently.
  • Added deterministic regression coverage for rapid 24h → 7d → 30d → all changes and for A → B → A reuse. Both tests failed against the prior implementation before the fix.
  • Correction 3 from my previous disposition is withdrawn. The Desktop retry loop covers operation_conflict; it does not recover a capture that outlives the reservation idle deadline and fails finalization through the untyped error path. The PR body now limits the retry claim to transient capacity conflicts. No timing-policy change is included here because the review classifies this as the current-store Path ④/P3; a follow-up should distinguish pending-capture lifetime from retained-snapshot idle lifetime.
  • The legacy-variant statement is also narrowed: filtered kind: "logs" has a production caller in latestRuntimeProbe; usage:buckets, loadAllBuckets, and kind: "buckets" are pre-existing unused paths. They are not deleted in this fix(runtime-host): serve one revision-consistent Usage snapshot #4058 remediation.

Verification on 9f9dd0a20:

  • Desktop suite: 2,240 passed, 0 failed
  • focused Usage feature scope suite: 9 passed, 0 failed
  • Desktop typecheck
  • Desktop main build
  • targeted Biome check
  • git diff --check
  • independent review: no Critical or Important findings

The requested 50k-row / six-minute manual Desktop exercise was not run locally; the rapid-switch scheduling path is covered deterministically by the new tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XXL Over 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(runtime-host): serve one revision-consistent Usage snapshot

2 participants