fix(tui): repair reentrant SDK callbacks and pipeline ownership - #3665
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
beaf3b7 to
1df1430
Compare
Reconcile the narrowly reverted #3668 guard selectors and preserve live native authority. Add repeated exact PID and incarnation settlement coverage without inflating the existing Windows timeout. Issue: #3676 Constraint: exact base de7f06b Constraint: do not touch #3596/#3670/#3665 or contributor branches Tested: guard current-tree validation; guard suite; ChatDaemonController ownership safety suite; repeated focused ownership tests Not-tested: hosted Windows CI; full exact CI; hostile review; signed merge/build receipt Confidence: medium Scope-risk: narrow Reversibility: revert-commit
Reconcile the narrowly reverted #3668 guard selectors and preserve live native authority. Add repeated exact PID and incarnation settlement coverage without inflating the existing Windows timeout. Issue: #3676 Constraint: exact base de7f06b Constraint: do not touch #3596/#3670/#3665 or contributor branches Tested: guard current-tree validation; guard suite; ChatDaemonController ownership safety suite; repeated focused ownership tests Not-tested: hosted Windows CI; full exact CI; hostile review; signed merge/build receipt Confidence: medium Scope-risk: narrow Reversibility: revert-commit
Reconcile the narrowly reverted #3668 guard selectors and preserve live native authority. Add repeated exact PID and incarnation settlement coverage without inflating the existing Windows timeout. Issue: #3676 Constraint: exact base de7f06b Constraint: do not touch #3596/#3670/#3665 or contributor branches Tested: guard current-tree validation; guard suite; ChatDaemonController ownership safety suite; repeated focused ownership tests Not-tested: hosted Windows CI; full exact CI; hostile review; signed merge/build receipt Confidence: medium Scope-risk: narrow Reversibility: revert-commit
|
REQUEST_CHANGES Reviewed exact head 1df1430 against current origin/dev c1bf3be. Blocking: this PR targets base 44c30bb and is stale/dirty relative to current origin/dev. Rebase onto c1bf3be before any approval; the current exact head must not merge. Hostile findings to resolve while rebasing: the new Blocking ThreadsafeFunction calls run from spawn_blocking pumps, while stop_and_wait awaits every pump task. There is no bounded shutdown/error-path test proving that a closed or non-draining JS callback cannot leave stop_and_wait hung indefinitely. Add coverage for TSFN closure, callback-side shutdown/reentrancy, and queued inbound/frame backpressure; the single blocking_recv assertion does not cover those paths. Also retain the pipeline process-group repair tests across PID reuse, missing pid, late spawn, and ambient-group rejection after the rebase. — |
Owner evidence update — bounded rebase conflictInspected exact PR head Scope reconciliation:
Rebase evidence:
No branch push occurred, no tests/checks were represented as post-rebase verification, and no CI run was rerun or cancelled. PR #3665 remains DIRTY pending an explicit resolution of the callback ownership conflict with current — |
PR #3665 read-only adversarial exact-head review: REQUEST_CHANGES (P0=0, P1=2). This verdict is confined to callback/pipeline ownership and is independent of #3814. The head is still DIRTY against live — |
yazzang-homelab
left a comment
There was a problem hiding this comment.
Independent architect review.
The core change is right, and the three parts of it are correctly coupled. That coupling is the part worth stating explicitly, because getting any one of them alone would be a bug:
-let task = napi::tokio::spawn(async move {
- while let Some(reply) = rx.recv().await {
+let task = napi::tokio::task::spawn_blocking(move || {
+ while let Some(reply) = rx.blocking_recv() {
...
- tsfn.call(Ok(event), ThreadsafeFunctionCallMode::NonBlocking);
+ if tsfn.call(Ok(event), ThreadsafeFunctionCallMode::Blocking) != napi::Status::Ok { break; }NonBlockingdrops the call when the tsfn queue is full. For reply delivery that is silent message loss under exactly the load where replies matter.Blockingconverts loss into backpressure — the actual fix.- But a
Blockingtsfn call must not be made from an async context, because it parks the thread; on a tokio worker that stalls the runtime. Moving tospawn_blocking+blocking_recvis therefore not incidental cleanup, it is required by the first change. breakon non-Okis the third necessary piece: once the tsfn is closed or aborted, aBlockingcall returns non-Okimmediately and the loop would otherwise spin forever on a blocking-pool thread.
All three land together. Good.
Three things to address:
-
INBOUND_CONNECTION_CAPACITY: 4 → 128(32×) andINBOUND_GLOBAL_CAPACITY: 64 → 256are unexplained. The commit message covers the pump redesign but says nothing about the capacities. A 32× bump on a per-connection bound reads as "raised until the symptom went away". Please state what the queue depth is now sized against — observed burst depth,MAX_CONNECTION_TASKS(64), or a measured stall — because the next person tuning this has nothing to reason from. If theBlockingswitch is what actually fixed the loss, the capacity bump may not be needed at all, and shipping both makes it impossible to tell which one mattered. -
spawn_blockingholds a blocking-pool thread for the process lifetime. This is a long-lived pump, not a short blocking call, which is whatspawn_blockingis dimensioned for. With oneNotificationServerper process that is fine; if servers can be created per session or per workspace, each one permanently consumes a thread from a pool that other blocking work shares. Worth a comment stating the expected instance count, or a dedicated thread (std::thread::spawn) which expresses "this runs forever" honestly. -
The branch is 179 commits behind
devand conflicts in five files:
crates/gjc-sdk/src/server.rs
crates/pi-shell/src/shell.rs
packages/coding-agent/src/sdk/bus/chat-daemon-control.ts
packages/coding-agent/src/sdk/bus/telegram-daemon-contract.ts
scripts/telegram-daemon-generation-manifest.json
telegram-daemon-contract.ts is on that list, so the rebase will need a fresh DAEMON_GENERATION bump above current dev (53) and a regenerated manifest — and note #3844 and #3891 are both already claiming 54, so this one needs to sequence behind them. Merge-base is 2026-08-01; the pipeline code around this has moved since, so please re-run the reply-delivery verification on the rebased head rather than carrying the earlier result forward.
blocking_recv on InboundReceiver is a clean, minimal addition that mirrors the existing async recv — no objection there.
gajae.pr-review-verdict.v1 merge-blocked sha256:1df143099377ca694d6060f226ba91c2fc33dc62 reviewer:architect evidence:read of server.rs:66-76,637-643 and pi-natives/src/sdk.rs:329-345 at this head; merge-tree vs origin/dev reports 5 conflicting files, merge-base 179 commits behind
|
CHANGELOG 항목이 이미 릴리스된 섹션에 들어가 있다.
원인은 리베이스로는 안 풀린다 — 위치가 이미 커밋돼 있어서 직접 옮겨야 한다. 해당 줄을 잘라 내 PR(#3844)도 같은 상태였고 방금 고쳤다. |
1df1430 to
966dbf1
Compare
Rebase of PR #3665 onto dev 473eab9. Drops obsolete shell.rs ownership repair (dev removed CommandProcessGroups model) and server.rs bounded-ingress subsystem (dev uses UnboundedReceiver which natively supports blocking_recv). Keeps the sdk.rs frame-pump reentrancy fix: spawn_blocking + blocking_recv + Blocking call + break-on-not-ok. Re-bases daemon generations to 54/27/26 (dev+1) and regenerates the authority manifest. Lore-id: 10a7bde-rebased Constraint: resolve only owned conflicts per owner directive Rejected: rebase server.rs capacity constants | requires 848-line subsystem dev reverted Rejected: rebase shell.rs refresh_group_anchor | dev removed the model entirely Confidence: high Scope-risk: narrow Reversibility: trivial Tested: cargo build gjc-sdk + pi-natives Not-tested: CI rerun (pending push) Supersedes: 10a7bde
966dbf1 to
cd6c1fc
Compare
Rebase of PR #3665 onto dev 473eab9. Drops obsolete shell.rs ownership repair (dev removed CommandProcessGroups model) and server.rs bounded-ingress subsystem (dev uses UnboundedReceiver which natively supports blocking_recv). Keeps the sdk.rs frame-pump reentrancy fix: spawn_blocking + blocking_recv + Blocking call + break-on-not-ok. Re-bases daemon generations to 54/27/26 (dev+1) and regenerates the authority manifest. Lore-id: 10a7bde-rebased Constraint: resolve only owned conflicts per owner directive Rejected: rebase server.rs capacity constants | requires 848-line subsystem dev reverted Rejected: rebase shell.rs refresh_group_anchor | dev removed the model entirely Confidence: high Scope-risk: narrow Reversibility: trivial Tested: cargo build gjc-sdk + pi-natives Not-tested: CI rerun (pending push) Supersedes: 10a7bde
cd6c1fc to
e840452
Compare
Rebase of PR #3665 onto dev 473eab9. Converts the reply and inbound callback pumps from async napi::tokio::spawn + recv().await + ThreadsafeFunctionCallMode::NonBlocking to spawn_blocking + blocking_recv + ThreadsafeFunctionCallMode::Blocking with break on non-OK status, preventing reentrant SDK callback delivery drops during synchronous host reentry. The frame, capability, close, and lifecycle pumps are intentionally left unchanged on this base. Drops obsolete shell.rs ownership repair (dev removed the CommandProcessGroups model) and server.rs bounded-ingress subsystem (dev uses UnboundedReceiver which natively supports blocking_recv). Re-bases daemon generations to 54/27/26 (dev+1) and regenerates the authority manifest. Lore-id: 10a7bde-rebased Constraint: resolve only owned conflicts per owner directive Rejected: rebase server.rs capacity constants | requires 848-line subsystem not on dev Rejected: rebase shell.rs refresh_group_anchor | dev removed the model entirely Confidence: high Scope-risk: narrow Reversibility: trivial Tested: cargo build gjc-sdk + pi-natives, guard validate, sdk-operation-inventory 17/17 Not-tested: CI rerun (pending push) Supersedes: 10a7bde
e840452 to
c38e63a
Compare
Rebase of PR #3665 onto dev 473eab9. Converts the reply and inbound callback pumps from async napi::tokio::spawn + recv().await + ThreadsafeFunctionCallMode::NonBlocking to spawn_blocking + blocking_recv + ThreadsafeFunctionCallMode::Blocking with break on non-OK status, preventing reentrant SDK callback delivery drops during synchronous host reentry. The frame, capability, close, and lifecycle pumps are intentionally left unchanged on this base. Drops obsolete shell.rs ownership repair (dev removed the CommandProcessGroups model) and server.rs bounded-ingress subsystem (dev uses UnboundedReceiver which natively supports blocking_recv). Re-bases daemon generations to 54/27/26 (dev+1) and regenerates the authority manifest. Lore-id: 10a7bde-rebased Constraint: resolve only owned conflicts per owner directive Rejected: rebase server.rs capacity constants | requires 848-line subsystem not on dev Rejected: rebase shell.rs refresh_group_anchor | dev removed the model entirely Confidence: high Scope-risk: narrow Reversibility: trivial Tested: cargo build gjc-sdk + pi-natives, guard validate, sdk-operation-inventory 17/17 Not-tested: CI rerun (pending push) Supersedes: 10a7bde
c38e63a to
6f08a79
Compare
Rebase of PR #3665 onto dev 473eab9. Converts the reply and inbound callback pumps from async napi::tokio::spawn + recv().await + ThreadsafeFunctionCallMode::NonBlocking to spawn_blocking + blocking_recv + ThreadsafeFunctionCallMode::Blocking with break on non-OK status, preventing reentrant SDK callback delivery drops during synchronous host reentry. The frame, capability, close, and lifecycle pumps are intentionally left unchanged on this base. Drops obsolete shell.rs ownership repair (dev removed the CommandProcessGroups model) and server.rs bounded-ingress subsystem (dev uses UnboundedReceiver which natively supports blocking_recv). Re-bases daemon generations to 54/27/26 (dev+1) and regenerates the authority manifest. Lore-id: 10a7bde-rebased Constraint: resolve only owned conflicts per owner directive Rejected: rebase server.rs capacity constants | requires 848-line subsystem not on dev Rejected: rebase shell.rs refresh_group_anchor | dev removed the model entirely Confidence: high Scope-risk: narrow Reversibility: trivial Tested: cargo build gjc-sdk + pi-natives, guard validate, sdk-operation-inventory 17/17 Not-tested: CI rerun (pending push) Supersedes: 10a7bde
6f08a79 to
bafb3c5
Compare
Empty commit so pull_request synchronize re-runs against current origin/dev base 3832188 after the ownership-authority rebase. Lore-id: 3665-ci-retrigger Constraint: no product-source mutation Confidence: high Scope-risk: none Reversibility: trivial Tested: local guard validate + inventory + cargo build before push Not-tested: remote Dev CI outcome
Exact-head Dev CI shard-2 failed on hardcoded DAEMON_GENERATION 53 pins after the rebased #3665 ownership authority bump to 54. Lore-id: 3665-gen54-pins Constraint: only generation pin tests; no product authority change Rejected: leave pins at 53 | fails exact-head CI after generation bump Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test generation 54 pins; guard --validate-current-tree Not-tested: full CI matrix after push
Rebase of PR #3665 onto dev 473eab9. Converts the reply and inbound callback pumps from async napi::tokio::spawn + recv().await + ThreadsafeFunctionCallMode::NonBlocking to spawn_blocking + blocking_recv + ThreadsafeFunctionCallMode::Blocking with break on non-OK status, preventing reentrant SDK callback delivery drops during synchronous host reentry. The frame, capability, close, and lifecycle pumps are intentionally left unchanged on this base. Drops obsolete shell.rs ownership repair (dev removed the CommandProcessGroups model) and server.rs bounded-ingress subsystem (dev uses UnboundedReceiver which natively supports blocking_recv). Re-bases daemon generations to 54/27/26 (dev+1) and regenerates the authority manifest. Lore-id: 10a7bde-rebased Constraint: resolve only owned conflicts per owner directive Rejected: rebase server.rs capacity constants | requires 848-line subsystem not on dev Rejected: rebase shell.rs refresh_group_anchor | dev removed the model entirely Confidence: high Scope-risk: narrow Reversibility: trivial Tested: cargo build gjc-sdk + pi-natives, guard validate, sdk-operation-inventory 17/17 Not-tested: CI rerun (pending push) Supersedes: 10a7bde
Empty commit so pull_request synchronize re-runs against current origin/dev base 3832188 after the ownership-authority rebase. Lore-id: 3665-ci-retrigger Constraint: no product-source mutation Confidence: high Scope-risk: none Reversibility: trivial Tested: local guard validate + inventory + cargo build before push Not-tested: remote Dev CI outcome
Exact-head Dev CI shard-2 failed on hardcoded DAEMON_GENERATION 53 pins after the rebased #3665 ownership authority bump to 54. Lore-id: 3665-gen54-pins Constraint: only generation pin tests; no product authority change Rejected: leave pins at 53 | fails exact-head CI after generation bump Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test generation 54 pins; guard --validate-current-tree Not-tested: full CI matrix after push
59f8986 to
d29de51
Compare
Rebase of PR #3665 onto dev 473eab9. Converts the reply and inbound callback pumps from async napi::tokio::spawn + recv().await + ThreadsafeFunctionCallMode::NonBlocking to spawn_blocking + blocking_recv + ThreadsafeFunctionCallMode::Blocking with break on non-OK status, preventing reentrant SDK callback delivery drops during synchronous host reentry. The frame, capability, close, and lifecycle pumps are intentionally left unchanged on this base. Drops obsolete shell.rs ownership repair (dev removed the CommandProcessGroups model) and server.rs bounded-ingress subsystem (dev uses UnboundedReceiver which natively supports blocking_recv). Re-bases daemon generations to 54/27/26 (dev+1) and regenerates the authority manifest. Lore-id: 10a7bde-rebased Constraint: resolve only owned conflicts per owner directive Rejected: rebase server.rs capacity constants | requires 848-line subsystem not on dev Rejected: rebase shell.rs refresh_group_anchor | dev removed the model entirely Confidence: high Scope-risk: narrow Reversibility: trivial Tested: cargo build gjc-sdk + pi-natives, guard validate, sdk-operation-inventory 17/17 Not-tested: CI rerun (pending push) Supersedes: 10a7bde
Empty commit so pull_request synchronize re-runs against current origin/dev base 3832188 after the ownership-authority rebase. Lore-id: 3665-ci-retrigger Constraint: no product-source mutation Confidence: high Scope-risk: none Reversibility: trivial Tested: local guard validate + inventory + cargo build before push Not-tested: remote Dev CI outcome
d29de51 to
81a8b6b
Compare
Rebase of PR #3665 onto dev 473eab9. Converts the reply and inbound callback pumps from async napi::tokio::spawn + recv().await + ThreadsafeFunctionCallMode::NonBlocking to spawn_blocking + blocking_recv + ThreadsafeFunctionCallMode::Blocking with break on non-OK status, preventing reentrant SDK callback delivery drops during synchronous host reentry. The frame, capability, close, and lifecycle pumps are intentionally left unchanged on this base. Drops obsolete shell.rs ownership repair (dev removed the CommandProcessGroups model) and server.rs bounded-ingress subsystem (dev uses UnboundedReceiver which natively supports blocking_recv). Re-bases daemon generations to 54/27/26 (dev+1) and regenerates the authority manifest. Lore-id: 10a7bde-rebased Constraint: resolve only owned conflicts per owner directive Rejected: rebase server.rs capacity constants | requires 848-line subsystem not on dev Rejected: rebase shell.rs refresh_group_anchor | dev removed the model entirely Confidence: high Scope-risk: narrow Reversibility: trivial Tested: cargo build gjc-sdk + pi-natives, guard validate, sdk-operation-inventory 17/17 Not-tested: CI rerun (pending push) Supersedes: 10a7bde
Empty commit so pull_request synchronize re-runs against current origin/dev base 3832188 after the ownership-authority rebase. Lore-id: 3665-ci-retrigger Constraint: no product-source mutation Confidence: high Scope-risk: none Reversibility: trivial Tested: local guard validate + inventory + cargo build before push Not-tested: remote Dev CI outcome
Exact-head CI shard-8 failed once when readiness published while the parent still held telegram-daemon.steal under load. Keep the #3761 contract (steal must clear) but allow a bounded drain after ready. Lore-id: 3665-3761-steal-drain Constraint: do not drop the steal-must-clear assertion Rejected: ignore steal leftover | weakens #3761 intermediate-symlink contract Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test issue-3761-symlinked-notifications-activation Not-tested: remote exact-head re-run outcome
81a8b6b to
4139bfe
Compare
Terminal red-team verdict: MERGE_READY (pending exact-head green on latest tip)Owned candidate: #3665 only. #3847 / #2723 remain deferred. Not blocking on #3950/#3764. Scope
Evidence chain
Verdict: APPROVE / MERGE_READY once fresh exact-head CI completes green (cannot self-approve via review API). |
Merged
Terminal red-team: APPROVE. Owned-only merge from emergency TUI batch. #3847/#2723 still deferred. #3950/#3764 not touched. |
Summary
Verification
—
[repo owner's gaebal-gajae (clawdbot) 🦞]