fix(android): keep cold-start gateway auto-connect from overriding explicit intents#101799
Conversation
…plicit intents NodeRuntime's init-time auto-connect now atomically claims the first gateway lifecycle intent (CAS 0->1) and stands down permanently when any explicit connect/disconnect/switch intent already exists, so a late discovery emission can no longer override a user decision. This was also the root cause of the CI-only flakes in GatewayBootstrapAuthTest (refreshGatewayConnection_* at :616/:627 and switchToUndiscoveredGateway* on PRs #101002/#101387/#101396). GatewaySession.Connection.sendRequestFrame now starts its response watcher with CoroutineStart.ATOMIC: disconnect teardown cancels connectionJob right after failPending(), and a DEFAULT-start watcher still queued for dispatch was cancelled without running, silently dropping the terminal UNAVAILABLE onError owed to fire-and-forget callers (root cause of the flaky GatewaySessionInvokeTest.disconnectReportsUnknownOutcomeForFireAndForgetRpc). Tests neutralize runtime background work (cancel+join the runtime scope) before arming the registry and scripting lifecycle steps, and add regression coverage for the auto-connect intent claim. Proof: refresh flake reproduced at iteration 1 under taskset -c 0,1 on a Blacksmith Linux box; post-fix 3 pinned stress rounds (120/150 looped iterations) plus full :app:testPlayDebugUnitTest (109 classes, 0 failures) and :app:ktlintCheck green on the same box.
openclaw-barnacle
Bot
added
app: android
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93e53bdede
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!gatewayLifecycleIntentSeq.compareAndSet(0L, 1L)) return | ||
| launchConnect(endpoint, explicitAuth = null) |
There was a problem hiding this comment.
Because compareAndSet(0L, 1L) only claims the sequence before launchConnect, an explicit lifecycle call can still increment the sequence in the gap before launchGatewayLifecycle() samples it. In that interleaving (auto CAS succeeds, user disconnects/connects and bumps the sequence, then this call captures the newer value), the auto-connect block passes the guard and reconnects to the saved endpoint, so cold-start discovery can still override the user's explicit intent. Pass the claimed intent into the guarded launch or recheck against the claimed value under the lifecycle lock.
Useful? React with 👍 / 👎.
|
Codex review: needs changes before merge. Reviewed July 7, 2026, 1:43 PM ET / 17:43 UTC. Summary PR surface: Other +68. Total +68 across 4 files. Reproducibility: yes. from source inspection: the PR head can still let an explicit intent increment gatewayLifecycleIntentSeq after the auto-connect CAS but before launchGatewayLifecycle samples it. I did not run Android tests in this read-only review. Review metrics: none identified. Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Land this after auto-connect uses the CAS-claimed lifecycle intent as the guard token, with a regression test that forces an explicit intent between the claim and guarded launch. Do we have a high-confidence way to reproduce the issue? Yes from source inspection: the PR head can still let an explicit intent increment gatewayLifecycleIntentSeq after the auto-connect CAS but before launchGatewayLifecycle samples it. I did not run Android tests in this read-only review. Is this the best way to solve the issue? No, not yet. The GatewaySession ATOMIC watcher is supported by the coroutine contract, but NodeRuntime still needs to bind the claimed auto-connect intent into the guarded launch. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 53d0c4ca5050. Label changesLabel justifications:
Evidence reviewedPR surface: Other +68. Total +68 across 4 files. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (1 earlier review cycle)
|
clawsweeper
Bot
added
proof: sufficient
|
Merged via squash.
|
…plicit intents (openclaw#101799) * fix(android): keep cold-start gateway auto-connect from overriding explicit intents NodeRuntime's init-time auto-connect now atomically claims the first gateway lifecycle intent (CAS 0->1) and stands down permanently when any explicit connect/disconnect/switch intent already exists, so a late discovery emission can no longer override a user decision. This was also the root cause of the CI-only flakes in GatewayBootstrapAuthTest (refreshGatewayConnection_* at :616/:627 and switchToUndiscoveredGateway* on PRs openclaw#101002/openclaw#101387/openclaw#101396). GatewaySession.Connection.sendRequestFrame now starts its response watcher with CoroutineStart.ATOMIC: disconnect teardown cancels connectionJob right after failPending(), and a DEFAULT-start watcher still queued for dispatch was cancelled without running, silently dropping the terminal UNAVAILABLE onError owed to fire-and-forget callers (root cause of the flaky GatewaySessionInvokeTest.disconnectReportsUnknownOutcomeForFireAndForgetRpc). Tests neutralize runtime background work (cancel+join the runtime scope) before arming the registry and scripting lifecycle steps, and add regression coverage for the auto-connect intent claim. Proof: refresh flake reproduced at iteration 1 under taskset -c 0,1 on a Blacksmith Linux box; post-fix 3 pinned stress rounds (120/150 looped iterations) plus full :app:testPlayDebugUnitTest (109 classes, 0 failures) and :app:ktlintCheck green on the same box. * chore(i18n): resync native inventory line positions after NodeRuntime edits
…plicit intents (openclaw#101799) * fix(android): keep cold-start gateway auto-connect from overriding explicit intents NodeRuntime's init-time auto-connect now atomically claims the first gateway lifecycle intent (CAS 0->1) and stands down permanently when any explicit connect/disconnect/switch intent already exists, so a late discovery emission can no longer override a user decision. This was also the root cause of the CI-only flakes in GatewayBootstrapAuthTest (refreshGatewayConnection_* at :616/:627 and switchToUndiscoveredGateway* on PRs openclaw#101002/openclaw#101387/openclaw#101396). GatewaySession.Connection.sendRequestFrame now starts its response watcher with CoroutineStart.ATOMIC: disconnect teardown cancels connectionJob right after failPending(), and a DEFAULT-start watcher still queued for dispatch was cancelled without running, silently dropping the terminal UNAVAILABLE onError owed to fire-and-forget callers (root cause of the flaky GatewaySessionInvokeTest.disconnectReportsUnknownOutcomeForFireAndForgetRpc). Tests neutralize runtime background work (cancel+join the runtime scope) before arming the registry and scripting lifecycle steps, and add regression coverage for the auto-connect intent claim. Proof: refresh flake reproduced at iteration 1 under taskset -c 0,1 on a Blacksmith Linux box; post-fix 3 pinned stress rounds (120/150 looped iterations) plus full :app:testPlayDebugUnitTest (109 classes, 0 failures) and :app:ktlintCheck green on the same box. * chore(i18n): resync native inventory line positions after NodeRuntime edits
1 participant
What Problem This Solves
Two Android JVM unit tests flaked repeatedly on Linux CI (
android-test-play) across unrelated PRs (#101002, #101387, #101396, observed 2026-07-06/07) while passing locally on macOS:GatewayBootstrapAuthTest.refreshGatewayConnection_reconnectsSavedManualEndpointAfterDisconnect—IllegalStateExceptionat thedesiredConnection(runtime, "nodeSession") ?: error(...)assertion (plus a sibling failure inswitchToUndiscoveredGatewayKeepsCurrentConnectionAndActiveGatewayin run 28830800141).GatewaySessionInvokeTest.disconnectReportsUnknownOutcomeForFireAndForgetRpc—TimeoutCancellationExceptionawaiting the fire-and-forgetonError(attempt-1 log of run 28847716276).Root causes:
NodeRuntime.initcollects gateway discovery on a background dispatcher and auto-connects the saved active gateway whenever that collector happens to run. On loaded CI runners it lands mid-test and races explicit connect/disconnect/refresh throughgatewayLifecycleIntentSeq(silent guarded-block drop) andgatewaySwitchMutex(async defer). This is also a real product hole: a late discovery emission could reconnect over an explicit user disconnect.GatewaySession.Connection.joinOwnedWork()cancelsconnectionJobright afterfailPending(). The fire-and-forget error watcher launched bysendRequestFrameis aDEFAULT-start child of that job; if still queued for dispatch it is cancelled without ever running, silently dropping the terminalUNAVAILABLE"Gateway disconnected before response" callback.Why This Change Was Made
autoConnectIfNeeded()now atomically claims the very first lifecycle intent (gatewayLifecycleIntentSeq.compareAndSet(0, 1)) and stands down permanently when any explicit connect/disconnect/switch intent already exists. Bothconnect()overloads share a newlaunchConnecthelper.sendRequestFramestarts its watcher withCoroutineStart.ATOMIC(stable@DelicateCoroutinesApiin kotlinx-coroutines 1.11.0, documented to start even when its job is already cancelled). SincefailPending()always precedes the cancel,await()surfaces the disconnect outcome without suspending.neutralizeColdStartAutoConnect), making the scripted sequences fully synchronous; two new regression tests pin the auto-connect intent-claim contract. Supersedes the poll-based stabilization from fix(android): stop saved gateway reconnect CI flake #101519 with a deterministic setup (its boundedwaitForDesiredConnectionremains for the final assert).User Impact
android-test-playCI lane stops flaking on these two tests.Evidence
taskset -c 0,1, with a seq probe proving the auto-connect interloper (seqBefore=3after only connect+disconnect).DEFAULTstart drops the terminal error when cancelled before first dispatch;ATOMICdelivers it. VerifiedDispatchedTaskexceptional-resume dominance andJobSupportfinal-cause selection in the 1.11.0 sources.:app:testPlayDebugUnitTest(109 classes, 0 failures),:app:ktlintCheckgreen; revalidated after rebasing onto latestmain.