Skip to content

fix(desktop): stop polling unavailable collaboration authority - #4527

Open
testikun wants to merge 6 commits into
apache:mainfrom
testikun:codex/issue-4522-collab-polling
Open

fix(desktop): stop polling unavailable collaboration authority#4527
testikun wants to merge 6 commits into
apache:mainfrom
testikun:codex/issue-4522-collab-polling

Conversation

@testikun

@testikun testikun commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #4522

Desktop now learns whether each Runtime Host exposes collaboration authority from the authenticated host.status boundary. The global turn-request inbox skips Owner Hosts that explicitly report no collaboration authority, while legacy/unknown Hosts are probed once and then remembered as unavailable when they return the expected unavailable result. Transient reconnect failures retain the existing retry behavior. Expected background operation_unavailable results are projected as an empty inbox so Electron does not log repeated rejected IPC handlers.

Implementation

  • Added an optional collaborationAuthority capability to Runtime Host status/diagnostics and propagated it through Desktop target identities.
  • Performed one bounded status observation when a Desktop candidate connects, then reused the capability during inbox polling.
  • Retained learned unavailable state when a legacy identity omits the optional capability.
  • Limited the empty-inbox fallback to the background poll path; session-scoped collaboration queries preserve their original failures and semantics.
  • Kept collaboration grants and permission semantics unchanged; only polling/read projection handles unavailable authority.

Verification

  • npm --workspace @maka/runtime-host run build
  • npm --workspace @maka/runtime-host run typecheck
  • npm --workspace @maka/desktop run build:test
  • 118 focused Runtime Host protocol and Desktop collaboration/manager tests passed.
  • npx biome check and npx biome lint passed for all changed files.
  • Desktop main-process typecheck passed. Full Desktop typecheck remains blocked by pre-existing workspace errors in apps/desktop/src/preload/runtime-host-session-catalog.ts and unrelated @maka/ui type drift under the available Node/npm environment.

Platform limitations

Hosts built against an older compatibility epoch must complete the normal upgrade/reconnect path before collaborating with this Desktop build. Legacy Hosts on the current epoch that omit the optional capability remain compatible: Desktop probes them once and remembers an expected unavailable collaboration result until a fresh Host identity/status observation says otherwise.

AI use

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

Tool(s) and scope: OpenAI Codex authored the implementation and regression tests.

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

@Phoenix500526 Phoenix500526 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 capability-based direction looks sound. I found three merge-readiness items below; I am leaving this as comments only, not an approval or merge request.

Comment thread packages/runtime-host/src/protocol/index.ts Outdated
Comment thread apps/desktop/src/main/runtime-host-collaboration-ipc-main.ts Outdated
@testikun
testikun force-pushed the codex/issue-4522-collab-polling branch from 1cf44b4 to b325755 Compare September 3, 2026 02:01
@testikun

testikun commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Addressed in b325755 (rebased onto the latest apache/main):

  • Advanced RUNTIME_HOST_COMPATIBILITY_EPOCH to 99 and documented the new host.status wire-shape rationale.
  • Kept the empty-inbox fallback limited to the background query, requiring the exact collaboration.turn-request.query operation plus operation_unavailable; scoped session queries still rethrow.
  • Added a polling/cache-boundary regression test proving an unknown legacy Host is queried once, caches authorityUnavailable=false, and is skipped on the next getPendingTurnRequests() poll.

Biome checks and the focused polling/cache tests pass locally. The full workspace build remains affected by pre-existing cross-package type drift under the available Node/npm environment.

@testikun
testikun force-pushed the codex/issue-4522-collab-polling branch 2 times, most recently from 4eea190 to 18f2821 Compare September 3, 2026 07:06
@testikun
testikun force-pushed the codex/issue-4522-collab-polling branch from 18f2821 to 5bb90b9 Compare September 3, 2026 07:21
Generated-by: OpenAI Codex
@testikun

testikun commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

已处理并重新提交。

  • 已合并最新 apache/main,解决合并冲突。
  • 保留 collaboration authority 的缓存/轮询修复。
  • Runtime Host compatibility epoch 更新为 106,避免与主线及其他开放协议变更冲突。
  • 本地针对性构建与测试通过(Runtime Host protocol、Desktop collaboration IPC、Host manager、preload polling:全部通过)。
  • 按约束未运行本机完整 Host 生命周期测试。

@Phoenix500526

Copy link
Copy Markdown
Contributor

The user-visible error stream from #4522 no longer reproduces on current main after #4557. That PR added a precise main-process fallback for the background collaboration.turn-request.query: when the Host returns operation_unavailable, the IPC handler now projects an empty inbox instread of rejecting, so Electron no longer prints the error every two secs.

The underlying polling behavior still remains on main, though. Desktop continues sending the query on every inbox interval. The unsupported result is simply handled quietly. This PR offers the more complete capability-bounary solution: new Hosts report whether collaboration authority is available, Desktop skips Hosts that explicity lack it, and legacy/unknown Hosts are probed once and remembered as unavailable while transient failures retain their retry semantics.

So #4557 has already resolve the noisy terminal symptom, while this PR would remove the unnecessary IPC and Runtime Host traffic that remains. @Astro-Han Could you take a look when you have times?

@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 at edaa9be. No P0 or P1. The main question is not correctness, it is whether the trade is worth making, so I want to put that first.

P2, and it decides the rest: after #4557 landed, the user-visible failure in #4522 is gone. @Phoenix500526 says so above and the code agrees, since this branch's base already contains the empty-inbox projection and this PR only adds a field to its return value. What is left is an optimization: one collaboration.turn-request.query every 2 seconds per Owner Host (use-turn-request-inbox.ts:32) that is known to fail. Worth weighing against that: the same connection already sends a host.status liveness probe every 2 seconds (connection.ts:87), so what gets removed is one small request on a connection that is already talking at that cadence.

The price is a new wire capability and a compatibility epoch bump, which means every deployed Host has to upgrade and reconnect before this Desktop build will talk to it. Against that, there is a smaller solution that gets the same result: runtime-host-collaboration-ipc-main.ts:95-114 already closes over a single client registered per target epoch, and already catches the exact error. Remember it there and short-circuit. That is one file, a handful of lines, naturally scoped per connection, and invalidated by reconnect for free, with nothing in packages/runtime-host and no epoch bump. If you also want the renderer-to-main IPC gone, keep the authorityUnavailable marker and the preload cache and still drop the protocol capability, the identity plumbing and the retention rule.

If the protocol capability is worth keeping for reasons beyond this poll, say so in the description, because as written a reader sees an epoch bump that invalidates every older Host in exchange for one empty query every 2 seconds.

One thing the capability derivation gets right, since it is not obvious: host-kernel.ts:842 derives it from accessAuthority, which is the same object that serves collaboration.turn-request.query at :786-792, so a false negative that silently kills a working inbox is not possible. The learned-negative cache is also bounded correctly: the preload scope key is hostId plus targetEpoch (preload.ts:276-278), so a reconnect on a new epoch drops what was learned rather than blacklisting a Host forever.

On the previous round: comment 1 (epoch plus rationale) and comment 2 (keep the narrower error.operation predicate) are both addressed. Comment 3 is not, see inline.

CI: there is no workflow run at all for this head. gh pr checks reports none, actions/runs?head_sha=edaa9be3 returns 0, and the commit has 0 check runs with a pending combined status. It is not action_required, there is simply no run. The green result you may be looking at is the previous head 5bb90b9d. The current head, the Merge apache/main into collab polling commit, is unverified. I could not determine from the API why it did not trigger.

Mergeability: CONFLICTING, and the only conflicting file is packages/runtime-host/src/protocol/index.ts. main is at epoch 109, so this needs 110 with the rationale carried over. That constant will keep conflicting, which is not your fault, but it does mean the rebase should be the last step before you ask for CI.

Evidence boundary: read only at edaa9be, no build, no test run, no Desktop launched. The redundancy claim in the ready-path comment rests on a static read of wait-for-ready.ts:34 and connection.ts:566; I did not observe that call.

AI-assisted review: drafted with Maka.

throw error;
}
if (result.kind === 'ready') {
const status = result.candidate.client.status;

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.

P2: this awaits an extra host.status round trip, up to 5 seconds, before target.hostId is set and the ready state is published. Every connect and every reconnect pays it, on a path that has nothing to do with collaboration, and a Host that is connected but slow stalls readiness for the full timeout.

It also looks redundant. The connect path already performs a host.status (client/wait-for-ready.ts:34), and connection.ts:566 hands that result to the onHostStatus observer, which this PR registers a few lines above at 987. So the same fact already arrives on its own, and this is a second acquisition path for it. Smallest fix: delete lines 1005 to 1011 and keep the observer.

P3 on line 1006: typeof status === 'function' guards a method that always exists on DesktopRuntimeHostClient. It is there to tolerate the fake candidate in runtime-host-desktop-manager.test.ts. Fix the harness rather than leaving a test-shaped guard in production code.

// replay one-shot join progress.
onConnectionPhase: (phase) => onConnectionPhase?.(phase),
...(refreshPeerRoutes ? {} : { refreshPeerRoutes: false }),
onHostStatus: (status) => {

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.

P2: this overrides onHostStatus unconditionally, after ...target.input. Before, it was only present when a caller supplied one (...(onHostStatus ? { onHostStatus } : {})).

That has a side effect the PR does not mention: connection.ts:833-839 returns early from #resetLivenessCheck when a status observer exists, so ongoing Session traffic no longer defers the liveness timer and host.status keeps firing on schedule at DEFAULT_LIVENESS_INTERVAL_MS = 2_000 (connection.ts:87). An idle connection already probed at that rate, so the increase is bounded, but the direction is the opposite of this PR's premise: to remove one 2 second query, every connection including the ones that do support collaboration gets a 2 second probe that traffic can no longer defer.

Resolve this together with the ready-path probe below. Either keep a main-process memo and register no observer at all, or keep the observer, register it only while the capability is unknown, and drop the explicit probe.

assert.equal(retainRuntimeHostCollaborationAuthority(true, false), true);
});

test('caches an unavailable legacy Host across polling calls', async () => {

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.

P3: this does not cross the boundary @Phoenix500526 asked about. The cache here is a local authority variable and a local mark closure, not runtimeHostMetadata and markRuntimeHostCollaborationUnavailable. Replace the callback passed at preload.ts:1418 with a no-op, or have it write the wrong scope key, and this test still passes, which is exactly the failure mode the request was meant to rule out.

Assert against the real pair instead: mark through markRuntimeHostCollaborationUnavailable, then read back through runtimeHostMetadataFor on the second poll.

}

/** Hosts with an explicit negative capability cannot answer collaboration queries. */
export function selectRuntimeHostCollaborationScopes<T extends RuntimeHostCollaborationScope>(

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.

P3: selectRuntimeHostCollaborationScopes has no production caller. A grep over apps/desktop/src finds only this definition and its test; the real filter is inlined at line 51. Either use it at line 51 or delete it with its test, so the exported surface does not carry a function that pins nothing.

return {
canRequestTurns: false,
requests: [],
authorityUnavailable: true,

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.

P3: authorityUnavailable is a cross-process contract field, but it exists only as an ad-hoc intersection at each end, here via satisfies CollaborationTurnRequestQueryResult & { authorityUnavailable: true } and at preload.ts:1404 via a cast. It is not declared in bridge-contract.d.ts, so renaming or dropping it on one side compiles clean on the other. Declare the query result shape once in the bridge contract and have both ends use it.


entries(): readonly RuntimeHostDesktopTargetState[] {
return [...this.#targets.values()].map((target) => target.state);
return [...this.#targets.values()].map((target) => ({

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.

P3: this spreads the generation's collaborationAuthority over target.state, but the state objects already carry the field from the constructors at 1227 to 1266 and from #projectState. One of the two writers is dead. Pick the state as the single carrier and drop the override here, or drop it from the state variants and keep the projection in one place.

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.

bug(desktop): stop polling unavailable collaboration authority

3 participants