-
Notifications
You must be signed in to change notification settings - Fork 3.1k
fix(onboard): retry transient forward readiness #8826
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -120,8 +120,12 @@ export function looksLikeUntrackedForward(diagnostic: string): boolean { | |
| * once OpenShell either keeps the attempt alive until the listener is ready or | ||
| * exposes a structured retryable outcome. Keep the fragments narrow so an | ||
| * unrelated SSH or gateway failure cannot enter the listener-retry path. | ||
| * OpenShell 0.0.101 can also reject a forward during the sandbox readiness | ||
| * handoff. That command has already exited, so list polling cannot recover it; | ||
| * the retry wrapper below gives the OpenShell gateway a bounded settle interval. | ||
| */ | ||
| export function looksLikeForwardListenerStartFailure(diagnostic: string): boolean { | ||
| if (/\bsandbox is not ready\b/i.test(diagnostic)) return true; | ||
|
Comment on lines
+123
to
+128
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
rg -n -C 6 \
'sandbox is not ready|looksLikeForwardListenerStartFailure|runDetachedForwardStartWithRetries|Permission denied|gateway transport unavailable' \
srcRepository: NVIDIA/NemoClaw Length of output: 45490 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- implementation ---'
cat -n src/lib/onboard/forward-start.ts | sed -n '110,145p;490,550p'
printf '%s\n' '--- tests ---'
cat -n src/lib/onboard/forward-start.test.ts | sed -n '940,1050p;1080,1110p'
printf '%s\n' '--- exact diagnostic constants and related contracts ---'
rg -n -C 5 \
'OPENSHELL_SANDBOX_NOT_READY|sandbox is not ready|listener-start-failure|runDetachedForwardStartWithDiagnostics' \
src/lib/onboard src/lib/actions/sandboxRepository: NVIDIA/NemoClaw Length of output: 50371 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- diagnostic construction ---'
cat -n src/lib/onboard/forward-start.ts | sed -n '321,490p'
printf '%s\n' '--- focused listener-start tests ---'
rg -n -C 8 \
'listener-start-failure|access denied|no forward list result|sandbox readiness rejection|sandbox is not ready' \
src/lib/onboard/forward-start.test.ts
printf '%s\n' '--- deterministic matcher probe ---'
python3 - <<'PY'
import re
matcher = re.compile(r"\bsandbox is not ready\b", re.I)
cases = {
"completed command": (
"Error: code: 'The system is not in a state required for the operation's "
'execution\', message: "sandbox is not ready"'
),
"composite authentication diagnostic": (
'forward start failed: Permission denied (publickey); '
'previous attempt reported "sandbox is not ready"'
),
"composite gateway diagnostic": (
'gateway transport unavailable while handling sandbox is not ready'
),
"unrelated forwarding diagnostic": (
'forward start failed: local target unavailable; sandbox is not ready was '
'reported by a different operation'
),
}
for name, diagnostic in cases.items():
print(f"{name}: {bool(matcher.search(diagnostic))}")
PYRepository: NVIDIA/NemoClaw Length of output: 14602 Match the full completed-command readiness diagnostic in both retry checks. The current substring match classifies composite authentication or gateway diagnostics as retryable and applies the 5-second delay. Keep unrelated forwarding failures terminal, and add a negative retry test for a composite diagnostic. 🤖 Prompt for AI Agents |
||
| return /ssh exited before local forward listener opened|local forward listener did not open\b/i.test( | ||
| diagnostic, | ||
| ); | ||
|
|
@@ -178,6 +182,7 @@ function blockingSleepMs(ms: number): void { | |
| // supported OpenShell version either stops retaining persistent dead rows or | ||
| // exposes an atomic recovery operation. | ||
| const DEAD_FORWARD_GRACE_MS = 2_000; | ||
| const SANDBOX_READY_RETRY_SETTLE_MS = 5_000; | ||
|
|
||
| /** | ||
| * Build a `DetachedForwardSpawnRunner` that spawns the given argv as a | ||
|
|
@@ -502,6 +507,7 @@ export function runDetachedForwardStartWithRetries( | |
| options: DetachedForwardStartOptions = {}, | ||
| ): DetachedForwardStartOutcome { | ||
| const maxRetries = options.maxRetries ?? 3; | ||
| const sleepImpl = options.sleepMs ?? blockingSleepMs; | ||
| let deadForwardRecoveryAvailable = true; | ||
| const isPortListening = options.isPortListening ?? probeLocalPortListening; | ||
| const runAttempt = (): DetachedForwardStartOutcome => | ||
|
|
@@ -528,6 +534,11 @@ export function runDetachedForwardStartWithRetries( | |
| if (looksLikeForwardPortConflict(attempt.diagnostic)) { | ||
| beforeRetryCleanup(); | ||
| } | ||
| if (/\bsandbox is not ready\b/i.test(attempt.diagnostic)) { | ||
| // Keep the existing sandbox and port ownership intact while the | ||
| // OpenShell gateway finishes the readiness handoff. | ||
| sleepImpl(SANDBOX_READY_RETRY_SETTLE_MS); | ||
| } | ||
| standardRetries++; | ||
| } | ||
| attempt = runAttempt(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the settle delay occurs before the second spawn.
expect(sleep).toHaveBeenCalledWith(5_000)proves only that a matching call occurred. It does not prove that the call happened between the failed and successful spawn, or that the test used only one settle delay. Record the injected events and assertspawn-1,sleep(5_000),spawn-2; also assert thatsleepwas called once.As per path instructions, verify behavioral confidence at the public boundary rather than only the presence of a mock call.
Suggested test adjustment
📝 Committable suggestion
🤖 Prompt for AI Agents
Source: Path instructions