Repository navigation
fix(node-host): share concurrent skill-bin refreshes - #127113
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs maintainer review before merge. Reviewed September 6, 2026, 4:26 AM ET / 08:26 UTC. ClawSweeper reviewWhat this changesThe node host shares overlapping skill-binary refresh requests and adds regression coverage for cache expiry, failures, retries, and connection replacement. Merge readiness✅ Ready for maintainer review The fix remains necessary: current main and v2026.9.2 still duplicate overlapping refreshes. The updated patch preserves connection retirement and has no actionable correctness findings. Priority: P2 Review scores
Verification
How this fits togetherThe node host asks the Gateway which skill executables may qualify for automatic command approval, then resolves those executables locally. Its connection-owned cache feeds command-policy evaluation. flowchart TD
A[Concurrent node commands] --> B[Skill auto-allow policy]
B --> C{Cache fresh?}
C -->|Yes| F[Resolved executable entries]
C -->|No| D[Shared refresh request]
D --> E[Gateway skill discovery]
E --> F
F --> G[Command authorization]
H[Connection retirement] -->|Replace cache| C
Before mergeNone. Agent review detailsSecurityNone. PR surfaceSource +10, Tests +160. Total +170 across 2 files. View PR surface stats
Review metricsNone. Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Technical reviewBest possible solution: Keep refresh coalescing inside the connection-owned cache while preserving existing trust retirement, expiry, and failure behavior. Do we have a high-confidence way to reproduce the issue? Yes, from source: two commands entering a cold or expired cache before the first response each start a refresh on current main. The supplied baseline results corroborate this; this review did not execute tests. Is this the best way to solve the issue? Yes. Sharing the pending promise at the existing cache owner removes duplicate work without adding configuration, changing the protocol, or altering connection retirement. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 66c9bf264b86. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (8 earlier review cycles)
|
Keep connection-owned cache retirement while coalescing overlapping refreshes. Replace the empty-result burst with bounded cold, expired, failure, retry, and retired-connection cases using resolved binary entries and complete cleanup.
|
@clawsweeper re-review Updated head: 28cff0e. The original fix's ancestry is preserved while integrating main's connection-owned cache retirement. The replacement tests use two overlapping requests and resolved binary entries. On trusted main 554233e, four cases failed for duplicate refresh requests and the old-connection isolation control passed; cleanup completed normally. Fresh independent source review found no actionable P0–P2 findings. Candidate CI is pending; the PR body records the exact scope and proof limits. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Merged via squash.
|
Closes openclaw#127095 ## What Problem This Solves Fixes an issue where node-host users with skill-based command auto-allow enabled would trigger a separate Gateway skill scan for each overlapping command when the shared cache was cold or expired. ## Why This Change Was Made The connection's cache now shares one in-flight refresh. Cleanup checks the promise identity so an older waiter cannot clear a newer retry. The existing 90-second TTL, empty initial-failure result, same-Gateway stale-on-failure behavior, and whole-cache replacement on connection retirement remain intact. The tests use two overlapping calls and actual resolved binary entries. They cover cold and expired refreshes, warm reuse, shared failure and retry, and an old connection's pending result completing after its replacement starts. Deferred calls are released and the runtime is closed even when a regression assertion fails. ## User Impact Overlapping node commands share the Gateway's workspace discovery work while retaining the same skill-bin values and authorization behavior. A retired connection cannot supply skill trust to its replacement. ## Evidence Trusted main baseline: `26e86d29fa3a7e0e49ffb8999fa2afda5df4c9c5`. - The existing node-host runtime suite passed all 24 tests. - The new bounded regressions produced four expected failures on main: cold and expired callers started duplicate refreshes, on both success and failure paths. The pending-old-connection control passed. All deferred cleanup completed normally. - The baseline ran with an isolated empty home/state environment. Candidate execution used secretless same-repository CI. - [Exact-head CI run 34021605844](https://github.com/openclaw/openclaw/actions/runs/34021605844) passed at `28cff0ec8d3f28ab28ca1ab1811325aa4e141463`: all 29 runtime tests, including 5 new cases, and all 9 worker-runtime tests. The latter include real benign command authorization across same-Gateway reconnect, replacement within/after TTL, and replacement-refresh failure. - Fresh independent Codex review and the latest exact-head ClawSweeper review found no actionable findings. Production: +11/-1 lines; tests: +163/-3. Production growth is the single in-flight promise slot, identity-checked cleanup, and its invariant comment.
Closes #127095
What Problem This Solves
Fixes an issue where node-host users with skill-based command auto-allow enabled would trigger a separate Gateway skill scan for each overlapping command when the shared cache was cold or expired.
Why This Change Was Made
The connection's cache now shares one in-flight refresh. Cleanup checks the promise identity so an older waiter cannot clear a newer retry. The existing 90-second TTL, empty initial-failure result, same-Gateway stale-on-failure behavior, and whole-cache replacement on connection retirement remain intact.
The tests use two overlapping calls and actual resolved binary entries. They cover cold and expired refreshes, warm reuse, shared failure and retry, and an old connection's pending result completing after its replacement starts. Deferred calls are released and the runtime is closed even when a regression assertion fails.
User Impact
Overlapping node commands share the Gateway's workspace discovery work while retaining the same skill-bin values and authorization behavior. A retired connection cannot supply skill trust to its replacement.
Evidence
Trusted main baseline:
554233e269f1a8cb47157c3e17604bc2209a37d9.28cff0ec8d3f28ab28ca1ab1811325aa4e141463: all 29 runtime tests, including 5 new cases, and all 9 worker-runtime tests. The latter include real benign command authorization across same-Gateway reconnect, replacement within/after TTL, and replacement-refresh failure.Production: +11/-1 lines; tests: +163/-3. Production growth is the single in-flight promise slot, identity-checked cleanup, and its invariant comment.