fix(peer): stop snapshot watch busy loops - #4687
Conversation
jackwener
left a comment
There was a problem hiding this comment.
Approving at exact head 57da397238695c4716309e6ec06c092059eb3ce7. No findings.
borrow() never advances a cloned receiver's seen version, so every call had an unread update waiting and changed() returned immediately — the zero-delay loop. borrow_and_update() is the right primitive, and it is applied to both watches.
The two new tests are falsifiable. Reverting only those two calls back to borrow() and changing nothing else fails both of them, at the is_err() assertions on the ten-millisecond window (bindings.rs:817 and :839). Unmodified, both pass. So they pin this fix rather than restating it.
The interleaving those tests do not cover is still safe, and it is the one worth spelling out: if the sender publishes G+1 between borrow_and_update() and the changed() await, nothing is swallowed. borrow_and_update() advances seen only to the value at call time, so changed() sees a newer version and returns immediately, and the following borrow() yields G+1. Losing an update would require seen to have been advanced past G+1 before the wait, which this code cannot do. The tests exercise send-then-watch-then-send, not that interleaving, so this rests on the primitive's semantics rather than on them.
Other .borrow() calls on receivers in this crate are correctly left alone: bindings.rs:141,174 are one-shot snapshot getters where advancing the stored receiver's seen would mislead a concurrent watch; the engine.rs sites are watch::Sender::borrow(), not a receiver's. relay_anchor_store.rs already pairs changed() with borrow_and_update.
The claim of no throttling, no retry state and no parallel cache holds — the diff is those two calls plus the two tests, and generation remains the single change authority.
Evidence boundary: the RED/GREEN runs above are mine, on cargo +1.98.0, with the tree restored afterwards. The release-addon timings in the description (203/202/202 ms against 1/0/0 ms) are the author's; neither reviewer on this side reproduced them. Hosted checks had not reached a terminal state when I posted — this approval covers the review gate only, branch protection still requires them, and it should not be read as covering a check that later fails.
简体中文
在 57da397238695c4716309e6ec06c092059eb3ce7 上批准。没有发现。
borrow() 从不推进克隆接收器的 seen version,于是每次调用都有一条「未读更新」挂着,changed() 立即返回——这就是那个零延迟循环。borrow_and_update() 是这里正确的原语,而且两处 watch 都改到了。
那两个新测试是可证伪的。 只把这两处调用回退成 borrow()、其余一字不动,两个测试都会失败,失败点正是那两条十毫秒窗口的 is_err() 断言(bindings.rs:817 与 :839);不改动则两个都通过。所以它们钉的是这次修复本身,而不是复述它。
那两个测试覆盖不到的交错,同样是安全的,而且这一点值得写明:如果发送方在 borrow_and_update() 与 changed() await 之间发布了 G+1,不会有任何东西被吞掉。borrow_and_update() 只把 seen 推进到调用当时的值,因此 changed() 会看到更新的 version 并立即返回,随后的 borrow() 取到 G+1。要丢掉一次更新,必须在等待之前就把 seen 推过 G+1,而这段代码做不到。测试走的是「先发送、再 watch、再发送」,不是这个交错,所以这条结论依据的是原语语义,而不是那两个测试。
这个 crate 里其它接收器上的 .borrow() 被正确地保留了:bindings.rs:141,174 是一次性读快照的 getter,在那里推进 stored receiver 的 seen 反而会误导并发的 watch;engine.rs 的几处是 watch::Sender::borrow(),不是接收器的。relay_anchor_store.rs 本来就是 changed() 配 borrow_and_update。
「不加限流、不加重试状态、不加并行缓存」这句成立——diff 就是那两处调用加两个测试,generation 仍是唯一的变化权威。
证据边界:上面的 RED/GREEN 是我本人跑的(cargo +1.98.0),跑完已恢复工作区。描述里 release addon 的耗时(203/202/202 毫秒 对 1/0/0 毫秒)是作者的,这一侧两位审查者都没有复现。我发布时托管检查尚未进入终态——本批准只覆盖审查门禁,分支保护仍然要求它们通过,也不应被理解为涵盖了之后失败的检查。
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.
Generated-by: Codex
57da397 to
63100d8
Compare
Prevent cloned Tokio watch receivers from redelivering an already-observed generation in a zero-delay loop. Use wait_for for seen-version tracking, keep reachability notifications token-only, and retain the snapshot getter as the single route-data authority. No persisted-state or peer wire migration is required. Generated-by: Codex Generated-by: GLM-5.3-Flash (ZCode)
Summary
Stop the native reachability and connectivity watches from redelivering an already-observed generation in a zero-delay loop.
The watches now delegate seen-version tracking to Tokio
wait_for, using the explicit snapshot generation only as the caller's change token. Reachability notifications return that token instead of transporting a second full route snapshot; the publisher notification likewise carries no unused lease payload. The authoritative reachability getter remains the only route data path, and the redundant JavaScript snapshot copy/freeze is gone.Refs #4677
Verification
cargo test --manifest-path native/runtime-host-peer/Cargo.toml reachability_watch_waits_for_a_newer_generationfailed because the old native API returned a full snapshot instead of the generation token.cargo test --manifest-path native/runtime-host-peer/Cargo.tomlpasses 30/30.npm run typecheck -w @maka/runtime-hostpasses.npm run build -w @maka/core,@maka/storage,@maka/runtime, and@maka/runtime-host, followed bynpm run test:dist -w @maka/runtime-host, passes 1,670 tests with 12 documented skips.npm run lintandnpm run lint:runtime-host-peerpass.cargo fmt --manifest-path native/runtime-host-peer/Cargo.toml --checkpasses.npm run build:runtime-host-peerproduces the release addon.watchReachability(1, 200)calls waited 202 ms, 201 ms, and 202 ms and returned scalar generation 1. The shipped dev.16 addon returned in 1 ms, 0 ms, and 0 ms.Root cause
The endpoint retained a Tokio
watch::Receiverwhose seen version never advanced. Every API call cloned that stale receiver and calledchanged(). Once the sender had published generation 1, every clone reported the old update as unread even whenafterGenerationwas already 1, so the JavaScript maintenance loops immediately requested the same snapshot again. Repeated N-API conversion and reachability publication consumed the Desktop main process.AI use
Select exactly one:
Tool(s) and scope: Codex diagnosed the packaged-app regression, authored the Rust and TypeScript repair and regression tests, audited the adjacent notification and persistence paths, and ran the native, workspace, lint, build, and packaged-addon verification.
Checklist
Does this PR entail a change in behavior?