Skip to content

Recognise a running stack on the other loopback - #469

Closed
kevin9327 wants to merge 1 commit into
CopilotKit:mainfrom
kevin9327:fix/already-running-other-loopback
Closed

Recognise a running stack on the other loopback#469
kevin9327 wants to merge 1 commit into
CopilotKit:mainfrom
kevin9327:fix/already-running-other-loopback

Conversation

@kevin9327

Copy link
Copy Markdown
Contributor

Open the desktop app, set OpenBot up, and leave it running. Close the window and open it again. The
window says "Set up OpenBot" for a stack that is already up, and pressing Start answers:

Something is already listening on port 3001, which OpenBot uses for the API server. Stop it, or
change the port, and start again.

That is OpenBot's own server. There is nothing on screen that can stop it, because the window does
not believe it is running.

Why

A second window has no handles on a stack an earlier one raised, so already_running asks the
deployment itself: is there a stamp here, and does the API answer? The second half asked one address:

client.get(format!("http://127.0.0.1:{port}/api/capabilities"))

Everything else in the shell that asks about that same port accepts either loopback. answering_at
walks LOOPBACKS = ["127.0.0.1", "[::1]"], and its comment says why: "A process that binds one and
not the other is normal rather than broken: Node resolves localhost to ::1 and bun to
127.0.0.1, so which one a service ends up on depends on what started it." The readiness wait uses
it, app_url uses it to point the window at the app, and #453 brought port_already_taken in line
with it last night.

already_running is the one that was left behind, and it is the one that decides which screen a
person sees. So the two disagree in exactly the worst way: the screen says nothing is running, and
the guard in front of Start says the port is taken.

The fix

The decision moves into stack.rs, beside the function that already asks both addresses, and takes
the port as an argument so a test can use one nothing else on the machine holds. main.rs keeps the
#[tauri::command] and nothing else — 21 lines out, 4 in — which also removes the last hand-rolled
reqwest client from that file.

One behaviour change worth naming: answering_at's client has a 3s timeout where the deleted one
had 2s, and it can now make two attempts. The check runs once, in the background of the setup
screen's first render, and a refused connection comes back immediately, so this is only visible on a
machine where a firewall drops rather than refuses.

Reproduction

Against 1c7bd92 (origin/main), unmodified. A stamped deployment directory and a listener
answering 200 on [::1]:3001 and nowhere else, handed straight to the shipped already_running:

running 1 test
test repro::a_deployment_answering_on_the_other_loopback_is_seen_as_running ... FAILED

---- repro::a_deployment_answering_on_the_other_loopback_is_seen_as_running stdout ----
thread '...' panicked at src\main.rs:813:9:
the stack is answering on [::1]:3001 and the window was told nothing is running

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.01s

The same probe with the listener on 127.0.0.1 instead passes on main, so it is the address that
decides it and not the harness.

The four tests this PR ships, against stack::already_running. Put the one-address client back
into the new function and 1 of the 4 fails:

test stack::tests::a_stack_answering_on_the_other_loopback_is_still_running ... FAILED
thread '...' panicked at src\stack.rs:866:9:
the API is answering on [::1]:51083 and the window was told nothing is running

test result: FAILED. 91 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

The other three are the guard against over-correcting, and pass before and after: an answer on
127.0.0.1 is still running, a stamp with nothing answering is not, and a directory this app never
installed into is not — that last one is what stops the window navigating to a stranger's server.

With the fix:

test result: ok. 92 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 4.06s

main is 88; the four above are the difference.

Verification

cargo fmt --check                                     # clean
cargo clippy --all-targets -- -D warnings             # clean
cargo test --lib                                      # 92 passed, 0 failed

Run from desktop/src-tauri on Windows with Rust 1.98.0. cargo test --lib is what desktop.yml
runs, so these are covered there.

Note on the changelog

The entry goes at the top of ## Unreleased, which is the same line every open PR touching the
changelog inserts at. Say the word if this needs rebasing onto whatever lands first.

Opening the desktop window a second time asks `already_running` whether the
deployment it manages is up, so a person is shown OpenBot rather than a setup
screen for a stack that is already running. The question was asked at
`127.0.0.1` alone.

Every other check on that port accepts either loopback, because a process binds
whichever one its runtime resolved `localhost` to: `answering_at` (the readiness
wait and the window's own navigation) and `port_already_taken` both ask both. So
a server on `::1` was reported as nothing running -- the window offered to set up
a deployment that was already up, and Start then refused on a port held by
OpenBot itself, naming it as somebody else's process.

The decision moves next to `answering_at` in `stack.rs`, which is what asks both
addresses, and takes the port as an argument so a test can use one nothing else
holds. `main.rs` keeps only the command.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@davidmckayv

Copy link
Copy Markdown
Contributor

Closing this as overtaken by main. already_running was restructured since this opened: recognition now rests on the recorded process-ownership stamp (recorded_process_owns_port) rather than a loopback probe, and the both-loopback helper (answering_at, from #453) is already in the tree. The code this patches no longer exists in that shape. One residual worth a small follow-up if you want it: server_capabilities_answer still probes 127.0.0.1 only, so a server answering solely on ::1 would miss that liveness check even though the ownership stamp holds. Not this PR's shape anymore, but the observation is a good one. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants