Skip to content

fix(desktop): stabilize WorkHub projection checkpoints - #4210

Merged
Astro-Han merged 3 commits into
apache:mainfrom
Astro-Han:fix/4205-stabilize-workhub-projection
Aug 29, 2026
Merged

fix(desktop): stabilize WorkHub projection checkpoints#4210
Astro-Han merged 3 commits into
apache:mainfrom
Astro-Han:fix/4205-stabilize-workhub-projection

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Stabilize WorkHub reconstruction across transient Runtime Host reconnects without introducing another coordination authority.

  • Keep the resolved WorkHub projection mounted when the same default Runtime Host reports reconnecting or ready. Revoke it only when Host authority actually changes or becomes unavailable/removed.
  • Make WorkHub E2E checkpoints wait for the existing Runtime Host readiness projection and the durable WorkHub work-count projection, then assert the reconstructed terminal turn instead of a transient local result.
  • Scope the layout contract to the submitted WorkHub turn whose metadata it measures.

Fixes #4205

Root cause and architecture boundary

The renderer lifecycle treated every default Host profile event as an authority replacement. When the existing Host briefly reported reconnecting, the lifecycle revoked its resolved generation and replaced WorkHub with the loading surface, detaching actions at the reconstruction and correction checkpoints.

The E2E coverage compounded that lifecycle defect by treating a mounted WorkHub main and .workhub-result as readiness. The former remains mounted while coordination reloads; the latter is transient local feedback and can disappear when the durable delegation projection replaces it.

Runtime Host and Session remain the authority. This change only compares the Host identity encoded in the already-resolved desktop Session key: reconnects from that same Host preserve the projection, while a real default Host switch or unavailable/removed Host still revokes and re-resolves it. Latest main already owns active-target admission and idle recovery in the Runtime Host boundary via #4185; this PR does not duplicate or replace that work.

Why this is not a wait or retry fix

No fixed sleeps, suite retries, operation retries, timeout increases, parallel store, or second lifecycle were added. The E2E helper observes the existing default Runtime Host profile until its owner reports ready, then waits for the corresponding WorkHub work-count projection before interacting.

Verification

  • npm run build — passed
  • Related WorkHub, Runtime Host, and storage tests — 145/145 passed; the simplified exact-head lifecycle test passed 5/5
  • Focused WorkHub layout and reconstruction E2E with --repeat-each=10 --workers=1 — 40/40 passed; after the test-only simplification, reconstruction passed 12/12 and the final exact-head layout contract passed 20/20
  • Full Desktop E2E on latest main — 89 passed, 1 skipped; one unrelated new-task-draft-target project-picker test failed because its menu did not reopen. A bounded three-repeat rerun of that file was 5/6 with the same independent failure.
  • npm run typecheck — passed
  • npm run lint — passed
  • npm run format:check — passed
  • npm run check:asf-headers — passed
  • git diff --check — passed

AI use

Select exactly one:

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

Tool(s) and scope: Codex diagnosed the lifecycle/readiness boundary, implemented the production and E2E changes, and added the regression test. The commit carries the required Generated-by: Codex trailer.

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/M Under 500 readable lines label Aug 29, 2026
@Astro-Han
Astro-Han marked this pull request as ready for review August 29, 2026 18:51

@ARE404 ARE404 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.

Approving fix(desktop): stabilize WorkHub projection checkpoints at head bde188f8.

I reviewed the source fix (workhub-coordination-lifecycle.ts) and its test updates:

  • The change tracks resolvedHostId (the host the active WorkHub projection resolved to) and skips re-resolving when the same already-resolved default host emits a benign event (event.hostId === resolvedHostId with readiness not unavailable and not removed). This stops unnecessary re-checkpointing/flicker while still re-resolving on a genuine change: a different default host, or the current host becoming unavailable/removed.
  • Edge handling is sound: when resolvedHostId is undefined (no resolution yet) the guard is bypassed and behavior matches before; a default-host swap or host-loss still revokes and resolves.
  • The added parseDesktopSessionKey import is validly exported from shared/runtime-host-identity.ts; hostId is now included in WorkHubCoordinationHostChange.
  • test is green on this exact head (run 33268495079, 16m04s); 0 unresolved review threads.

No P0–P2, and nothing warrants an inline P3.

简体中文

批准 fix(desktop): stabilize WorkHub projection checkpoints,head bde188f8
审查源码修复(workhub-coordination-lifecycle.ts)及其测试改动:该变更记录 resolvedHostId(当前 WorkHub 投影解析到的 host),当同一个已解析的默认 host 发出良性事件时(event.hostId === resolvedHostId 且 readiness 非 unavailable、未 removed)跳过重新解析——避免无谓的重新建点/闪烁;而真正变化时仍会重解析:默认 host 换人、或当前 host 变不可用/被移除。边界处理合理:resolvedHostId 为 undefined(尚未解析)时走旧逻辑;换默认 host 或 host 丢失仍 revoke+resolve。新增的 parseDesktopSessionKey import 在 shared/runtime-host-identity.ts 有合法导出;hostId 已加入 WorkHubCoordinationHostChangetest 在 exact head 绿(run 33268495079,16m04s);0 未解决线程。无 P0–P2,也无值得行内的 P3。

@Astro-Han
Astro-Han marked this pull request as draft August 29, 2026 18:57
@Astro-Han
Astro-Han marked this pull request as ready for review August 29, 2026 18:58

@zhiiw zhiiw 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 at exact head 29480dda (verified unchanged at review time). The head gained a second commit during review (test(desktop): remove duplicate WorkHub checkpoints); git range-diff confirms the original fix commit is patch-identical, and I verified the follow-up separately — see the last bullet.

Lifecycle fix, verified rather than assumed:

  • The guard is fail-safe in the right direction: the renderer keeps the resolved projection only when the event's hostId equals the identity parsed out of the already-resolved Session key. I traced the publisher in runtime-host-boot.ts: wire events carry hostId whenever the manager knows it (ready events take it from the live candidate; reconnecting/unavailable from the target's last known value). An event without a known hostId can never match, so unknown identity falls through to the old revoke-and-resolve path rather than accidentally pinning a stale projection.
  • The retained behaviors are the right ones: unavailable and removed always revoke; a different Host's ready revokes and re-resolves; the stale-resolution generation guard is untouched. The semantics deliberately key authority to the Host (not the profile): two default profiles pointing at the same Host share the same coordination Session key, so keeping the projection across that swap is consistent.
  • E2E readiness now observes authority, not pixels: waitForWorkHubReady waits on the default Host's readiness === 'ready' snapshot plus the durable work-count projection, replacing assertions on a mounted main (which stays mounted during reload) and on .workhub-result (transient local feedback). The layout contract now measures the specific submitted Turn it intends to measure instead of whichever result happened to be in the DOM.
  • The follow-up dedup is accurate: the deleted standalone regression test ("keeps its resolved projection while the same default Host reconnects") is subsumed by the updated second lifecycle test, which fires a same-Host reconnecting event after resolution and asserts no new resolve occurs. The guard is a single condition over all non-unavailable readiness values, so pinning it on reconnecting covers the mechanism. No assertion was lost.

Executed on a real Windows machine at this head: clean rebuild of the workspace libs (including the install-time model-metadata generation this head now requires) and Desktop main, then the WorkHub coordination lifecycle suite 5/5 — including the updated authority-change sequence that pins the same-Host reconnect behavior.


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

简体中文

生命周期修复,全部核实而非假设:守卫的失效方向是对的——只有事件的 hostId 与「已解析 Session key 里编码的 Host 身份」一致才保留投影;发布端(runtime-host-boot)在已知时恒带 hostId,未知则恒不匹配、落回旧的 revoke+resolve 路径。unavailable/removed 恒撤销,异 Host ready 恒重解析,generation 防陈旧守卫未动;权威键是 Host 而非 profile,与协调 Session key 的结构一致。E2E 改为观察 ready 快照 + 持久 work-count 投影,不再把「main 还挂着」当就绪。后续去重提交核实为真重复(被删的专项测试被更新后的序列测试涵盖,机制是单条件)。本机真 Windows 干净重建(含本 head 要求的安装期元数据生成)+ 生命周期套件 5/5。

Comment thread apps/desktop/e2e/workhub-layout.spec.ts Outdated

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I reviewed this at 29480dda2eb85b66384fd46337ff4ee826bafc95. No P0, P1, P2, or P3.

The renderer used to treat every default Host profile event as an authority replacement. A brief reconnecting from the same Host revoked the resolved WorkHub generation and swapped in the loading surface. The change keeps that generation when the already-resolved desktop Session key's hostId still matches, and the event is not unavailable or removed. A real default Host switch still revokes and re-resolves. Unavailable/removed still reports failure instead of spinning forever. Reconnect events from the Desktop Host manager include hostId once a candidate has been ready, which is the path this skip needs.

The E2E helper now waits for the default Host snapshot to be ready and for the durable work-count projection, then asserts the reconstructed terminal turn instead of a transient .workhub-result. That matches the production boundary: Host and Session stay the authority; this layer only decides when to keep or drop the already-resolved projection. Hosted test is green on this head, including Desktop e2e. MERGEABLE. This is a bugfix; I am not merging it.

简体中文

我审的是 29480dda2eb85b66384fd46337ff4ee826bafc95。没有 P0/P1/P2/P3。

同一默认 Host 短暂 reconnecting 时不再拆掉已经解析好的 WorkHub 投影。Host 真的换了、不可用、或被移除时仍会撤销并重新解析。E2E 改成等 Host ready 和 durable 工作计数,再断言重建后的终态,不再盯瞬时 .workhub-result。hosted test 绿了。这是 bugfix,我不合入。


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

@hqhq1025 hqhq1025 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.

Approved at exact head 29480dda2eb85b66384fd46337ff4ee826bafc95. I found no P0-P3 issues.

The production change is narrowly scoped and matches the ownership boundary: workhub-coordination-lifecycle.ts records the Host identity encoded in the resolved Coordination Session, preserves the mounted projection only for non-terminal events from that same Host, and still revokes on unknown/different Host identity, unavailable, or removed. I traced the event producer through runtime-host-boot.ts and runtime-host-desktop-manager.ts; reconnect events retain the target's Host identity while the target epoch remains stable, and a genuine target replacement/removal emits the revocation path. The existing generation check still rejects stale asynchronous resolutions.

The E2E changes wait on the default Host's authoritative ready snapshot plus WorkHub's durable work-count projection, scope selectors to the intended turn, and assert the reconstructed terminal state. I do not consider the existing P2 comment blocking: the layout regression's mutation-sensitive invariant is that the button contains its two-line metadata (buttonContainsProject), which failed before the original height: auto fix. .workhub-result is intentionally transient and optional after durable delegation replaces local feedback; because both elements remain in normal document flow, containing the metadata within the button is the non-vacuous overlap contract.

Review conclusions:

  • Optimal for this scope: yes; one Host identity field removes the false authority transition without adding another lifecycle or retry.
  • Deletable production code: none introduced or exposed.
  • Deletable/replaceable tests: the follow-up commit correctly removes duplicate readiness and same-Host checkpoints while retaining the behavior in the authority-change sequence.
  • Deeper refactor: not required.
  • Ready to merge: yes at this exact head.
  • Residual risk: timing-sensitive behavior always retains some platform variance, but exact-head hosted CI and the affected local suites are green. No protocol, storage, migration, or security boundary changed.

Validation on this exact head: Desktop typecheck; 85 WorkHub unit/integration tests; 4 targeted Electron E2E tests under Xvfb; GitHub test run 33269619737 (success, 13m25s); merge-ref tree equals the head tree; zero new code changes after the head refresh.

Codex-assisted review performed under the maintainer-approved review workflow.

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

@Astro-Han
Astro-Han merged commit 593efb9 into apache:main Aug 29, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(desktop): WorkHub reconstruction E2E fails at unstable projection checkpoints

6 participants