fix(runtime-host): serve revision-consistent Usage snapshots - #4068
fix(runtime-host): serve revision-consistent Usage snapshots#4068Sun-GLiang wants to merge 23 commits into
Conversation
…shot-consistency # Conflicts: # packages/runtime-host/src/protocol/index.ts
Astro-Han
left a comment
There was a problem hiding this comment.
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.
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
…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
left a comment
There was a problem hiding this comment.
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。
…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
|
Review disposition for 3415280: Implemented:
I did not apply the remaining suggestions verbatim:
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
|
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
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
left a comment
There was a problem hiding this comment.
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)
- The PR body still says epoch "101" — stale (actual 113 = main 112 + 1; code and commit message are correct).
- Epoch-113 merge-order dependency: #4068 shares 113 with #3299 and #4713 — whoever merges first takes the slot, the rest must rebase.
- 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 级改动,合并由人类定,注意纪元顺序。
|
Disposition for review 5122230496:
|
|
Updated the branch against
Post-merge verification:
The Website suite is not reported as passing locally because the shared worktree |
Astro-Han
left a comment
There was a problem hiding this comment.
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 维持。功能改动,合并由人类定。
|
Correction and disposition for follow-up review
CI |
Keep main's test-tier migration by retiring the WorkHub geometry E2E after its assertion moved to Storybook. Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
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()fixesidleExpiresAtat reservation time and capture does not extend it (usage-snapshot-cache.ts:129-139); after expiryfinalizereturnsundefined,#startUsageSnapshotthrows a plainError(usage-pricing-coordinator.ts:355),#mapReadFailurelands inunknownand rethrows, and the Desktop retry loop only coversoperation_conflict. Path ④ on today's stores, so P3, but the sentence in the body should go. usage:logs/usage:bucketsIPC handlers,loadAllBucketsandkind: 'buckets'are not exposed by the preload onmainand have no other caller;kind: 'logs'is still used bylatestRuntimeProbe. Pre-existing, not for this PR, but "legacy variants cannot be deleted" is only true oflogs.
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.
|
Disposition for review
Verification on
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. |
Summary
finallypath, preserve the latestmainsession-title hydration with bounded concurrency, and raise the merged Runtime Host compatibility epoch to 118Fixes #4058
Verification
9f9dd0a20: 2,240 passedgit diff --checkAI use
Select exactly one:
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
Does this PR entail a change in behavior?