Skip to content

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

Open
Sun-GLiang wants to merge 15 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 15 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
  • 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 101
  • cover five overlapping starts, idle and hard expiry, wrong-owner access, disconnect cleanup, failed Desktop loads, and protocol correlation

Fixes #4058

Verification

  • full Runtime Host suite: 1,620 passed; 12 skipped
  • full Desktop suite: 2,011 passed
  • full Storage suite: 1,087 passed; 8 skipped
  • npm run build
  • npm run typecheck
  • npm run format:check
  • npm run lint
  • protocol epoch guard: 100 -> 101
  • independent review: no Critical, Important, or Minor findings
  • git diff --check

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 Over 1000 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

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

Labels

effort/XL Over 1000 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