Skip to content

fix(desktop): bound reconnectable read retries - #3847

Closed
me2seeks wants to merge 2 commits into
apache:mainfrom
me2seeks:fix/3458-desktop-retained-root
Closed

fix(desktop): bound reconnectable read retries#3847
me2seeks wants to merge 2 commits into
apache:mainfrom
me2seeks:fix/3458-desktop-retained-root

Conversation

@me2seeks

@me2seeks me2seeks commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Summary

Reconnectable Desktop IPC reads could wait across an unlimited number of Runtime Host candidates. During prolonged connection flapping, new renderer refreshes could therefore leave additional request arguments and promises rooted in the main process even after each candidate was cleaned up.

Give each reconnectable read one 15-second replacement window shared across every candidate it visits. Reads still recover transparently when a replacement arrives inside that window; after it expires, the router rejects the request and releases its waiter. Commands, reconciled controls, target fencing, and Host reconnect scheduling are unchanged.

Refs #3458

Verification

  • npm --workspace @maka/desktop test — 1,518 tests passed
  • npm run lint
  • npm run format:check
  • npm run build
  • npm run typecheck
  • npx knip --workspace apps/desktop
  • npx knip --workspace packages/ui

Root cause evidence

Local forced-GC probes did not find linear retention in retired candidate graphs, Session restoration, UDS clients, or capability publication. The reconnecting IPC router did retain all 200 pending read argument markers while their target epoch remained active. The regression tests cover both a missing replacement and continuously failing replacements, including the case where each new generation is already installed and could otherwise bypass a per-wait timeout.

The deterministic retained-root proof and regression coverage make this change reviewable on its own. The Windows/Electron heap comparison under prolonged Runtime Host flapping remains tracked by #3458 before treating this path as the only contributor to the reported OOM or closing the issue.

AI use

Select exactly one:

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

Tool(s) and scope: Codex — root-cause investigation, implementation, tests, and pull request wording.

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

@me2seeks
me2seeks marked this pull request as ready for review August 26, 2026 03:08

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

I reviewed this head and found blocking process and non-blocking code issues.

[Standards Hard] Intentionally remaining follow-up but not Draft

Body lists unchecked task "heap convergence under prolonged flapping" but PR is Ready — template requires Draft when remaining work intentionally remains. Fix: keep Draft or move to separate issue.

[P3] 15s window uses wall clock

reconnecting-ipc-main.ts:262-266 uses Date.now() for deadline — clock rollback extends timeout. Fix: use performance.now().

Hosted test: SUCCESS (32925139009).

简体中文流程上应为 Draft,时序上建议用高精度时钟。

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.

@me2seeks

Copy link
Copy Markdown
Contributor Author

Thanks for catching both. I removed the unchecked follow-up from the PR checklist and left the Windows/Electron heap comparison tracked by #3458, since this PR does not claim the bounded read path is the only OOM contributor. I also switched the shared read deadline to performance.now() and updated the regression test to roll the wall clock backward while the monotonic clock advances. Desktop tests pass 1,518/1,518, with lint, format, and typecheck passing as well.

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

I reviewed this head and found no blocking issues.

Monotonic clock replacement deadline now shared correctly across failing candidates; hosted test: SUCCESS (32928851335).

No P0-P3.

简体中文该头无阻断。

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.

Keep the shared reconnectable-read window independent of wall-clock adjustments, and cover rollback while replacements keep failing.

Generated-by: Codex
@M4n5ter
M4n5ter force-pushed the fix/3458-desktop-retained-root branch from 3e62780 to 3961c6d Compare August 26, 2026 09:57
@M4n5ter

M4n5ter commented Aug 26, 2026

Copy link
Copy Markdown
Member

#3917 has merged with the retained-waiter fix from this PR folded into one invocation-wide monotonic replacement deadline. That implementation also covers initial handler absence and reconciliation without creating separate timeout authorities. The relevant authorship credit is preserved in the merged commit trailers.

I am closing this PR because #3917 supersedes it.

简体中文

#3917 已经合并,并将本 PR 的 waiter 保留问题修复收敛到一个覆盖整次调用的单调 replacement deadline。这个实现也统一覆盖了初始 handler 缺席与 reconciliation,不再维护多套超时权威。相关作者署名已保留在合并提交的 trailer 中。

因此,本 PR 已被 #3917 取代,我现在将其关闭。


Posted by an automated review agent operated by @M4n5ter. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.

简体中文

本条评论由 @M4n5ter 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。

@M4n5ter M4n5ter closed this Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants