Skip to content

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

Merged
Astro-Han merged 6 commits into
apache:mainfrom
testikun:codex/issue-4522-collab-polling
Sep 6, 2026
Merged

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

Conversation

@testikun

@testikuntestikun commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Stop repeated background collaboration inbox polling after a Runtime Host reports the expected operation_unavailable result.

  • The main-process IPC handler remembers the negative result for the lifetime of the registered Host candidate and returns an empty inbox without querying the Host again.
  • The preload remembers unavailable owner scopes by hostId and targetEpoch, so subsequent polls skip the IPC call; reconnect/identity changes naturally clear the cache.
  • Session-scoped collaboration queries keep their existing error behavior.

Design

This keeps the existing Runtime Host wire protocol unchanged: no host.status capability field, compatibility-epoch bump, target identity expansion, or Runtime Host API surface is required. A new candidate registration resets the main-process memo, and a new target epoch invalidates the preload scope cache.

Verification

  • Node 24 node --test apps/desktop/dist/main/__tests__/runtime-host-collaboration-ipc-main.test.js — 2 passed.
  • npm --workspace @maka/desktop run build:preload — passed.
  • Biome check passed for the three changed source/test files.
  • Ablation: removing the main-process memo made the second-query regression fail; the memo was restored.
  • Full Desktop main build is currently blocked by pre-existing mainline locale/type drift outside this PR.

Refs #4522

AI use

  • Generative tooling made a substantive contribution

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

@Phoenix500526Phoenix500526 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 threadpackages/runtime-host/src/protocol/index.ts Outdated
Comment threadapps/desktop/src/main/runtime-host-collaboration-ipc-main.ts Outdated
@testikun
testikunforce-pushed the codex/issue-4522-collab-polling branch from 1cf44b4 to b325755CompareSeptember 3, 2026 02:01
@testikun

Copy link
Copy Markdown
ContributorAuthor

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
testikunforce-pushed the codex/issue-4522-collab-polling branch 3 times, most recently from 18f2821 to 5bb90b9CompareSeptember 3, 2026 07:21
@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-HanAstro-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.

Comment threadapps/desktop/src/main/runtime-host-desktop-manager.ts Outdated
Comment threadapps/desktop/src/main/runtime-host-desktop-manager.ts Outdated
Comment threadapps/desktop/src/preload/runtime-host-turn-request-inbox.ts Outdated
Comment threadapps/desktop/src/main/runtime-host-collaboration-ipc-main.ts Outdated
Comment threadapps/desktop/src/main/runtime-host-desktop-manager.ts Outdated
@testikun
testikunforce-pushed the codex/issue-4522-collab-polling branch 2 times, most recently from f153c21 to 0c5adb4CompareSeptember 4, 2026 08:52
@testikun
testikunforce-pushed the codex/issue-4522-collab-polling branch from 0c5adb4 to 7b1bda7CompareSeptember 5, 2026 02:08

@Astro-HanAstro-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 7b1bda7. This is now the shape from the last round: no wire capability, no epoch, the memo lives in the IPC closure that is created per candidate connection (runtime-host-desktop-candidate.ts:833), so a reconnect resets it for free. The main-process test crosses the boundary @Phoenix500526 asked for: second query, zero client calls.

One P3, and it is a deletion. The preload cache (preload.ts:276, :1468-1486) and the authorityUnavailable marker are a second copy of the same fact, keyed by hostId + targetEpoch. Reconnect keeps the epoch, and the key is only dropped when the scope is observed non-ready between two 2-second polls; a reconnect that completes inside one poll gap to an upgraded Host leaves the preload skipping it until Desktop restart. The main-process memo alone has none of that, and removing the preload half also removes the ad-hoc CollaborationTurnRequestQueryResult & { authorityUnavailable?: true } intersection that is not declared in bridge-contract.d.ts. That is −15 lines and the PR is one file plus its test.

Approving as-is at 7b1bda7 with that noted; if you drop the preload cache, ping me and I will re-check the new head.

AI-assisted review: drafted with Maka; I verified the IPC registration lifetime and the scope-key invalidation myself.

@testikun

Copy link
Copy Markdown
ContributorAuthor

Applied the requested P3 cleanup in b4e530f:

  • removed the preload unavailable-scope cache and the ad-hoc authorityUnavailable result intersection
  • kept the main-process per-candidate memo and its regression assertion that the client is queried once
  • typecheck, main/preload builds, and the focused 2-test suite pass

@Astro-Han please re-check the new head.

@github-actionsgithub-actionsBot added effort/S Under 100 readable lines and removed effort/M Under 500 readable lines labels Sep 5, 2026

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

Reviewed at b4e530fdbe80fc1e502b19c96f48f7ebdcb7d0b0. The implementation is sound; I found no runtime correctness issue.

The negative result is cached only inside the collaboration IPC registration for one Runtime Host candidate. A replacement candidate creates a fresh registration and probes again. Only a background query receiving the exact collaboration.turn-request.query / operation_unavailable error sets the memo, while Session-scoped queries keep their existing error behavior. The focused two-test suite passed locally, and an additional production-module probe confirmed both properties: repeated calls use one Host query within a registration, and a fresh registration can recover to an available result. The exact-head hosted test check passed. A synthetic merge with current main (8c52a835c35bb3bd8a5b5512644e1fedcf5a02fc) was clean (34f0006820204a78f79f73207e218dd3329a178f).

[P2] Please refresh the PR record and AI attribution before merge. The current body still says that preload caches unavailable scopes by hostId and targetEpoch, that a new target epoch invalidates that cache, and that three source/test files changed. Head b4e530fdb removed the preload cache and the final diff is only the main-process handler plus its test, so those statements now describe code that no longer exists. In addition, the body says OpenAI Codex authored the implementation and regression test, but commit 7b1bda761 introduced the current memo and its regression assertions without the Generated-by: Codex trailer required by CONTRIBUTING.md. Please update the description to document the single main-process memo and amend every AI-authored affected commit to carry the required trailer.

The code path itself is ready after those contribution-record fixes. Please ping me after the amended head is green so I can bind the final review to it.


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

@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks for the cleanup! The current main-process memo looks right; I have no further code changes to request.

Just two finishing touches, consistent with the latest review: update the PR description to remove the old preload/hostId/epoch cache design, and add the required Generated-by: Codex trailer to commits containing substantive Codex contributions (including 7b1bda761). Please retain those trailers when squashing.

AI-assisted review with Codex.

@Astro-Han
Astro-Han merged commit a03dd57 into apache:mainSep 6, 2026
1 check passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/SUnder 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@testikun@Phoenix500526@Astro-Han@jackwener