Make the OIDC loopback wait cancellable so Ctrl+C exits

stephen commented

2026-09-06 15:34:42 +00:00

Owner

Fixes #257.

Fixed and Tested

Ctrl+C during fj auth login now exits:
- LoopbackServer::wait_for_code moved off spawn_blocking onto tokio::net::TcpListener. The accept loop, the request-line read, and the response write are async, so cli::run's SIGINT select actually cancels the wait and the runtime drops with nothing outstanding.
- The overall 300s budget moved from a hand-rolled deadline check into a tokio::time::timeout around the loop. Same semantics, and it now also covers connection handling.
- Covered by cancelling_wait_for_code_does_not_stall_runtime_shutdown.
A stalled connection no longer wedges the sign-in:
- Connections are handled one at a time and the request-line read was unbounded, so a socket that connected and then sent nothing (a browser speculative preconnect, a port scanner) blocked the loop forever and the real redirect behind it was never accepted.
- Bounded at REQUEST_LINE_TIMEOUT (10s), after which the connection is treated as a stray hit and the loop goes back to accepting.
Cargo.toml: declared the tokio net and time features this code now uses directly. Both were already present through feature unification; naming them keeps the build honest if a dependency stops pulling them in.

On the regression test

Asserting on runtime shutdown latency rather than on awaiting the future is deliberate. Awaiting wait_for_code looks identical either way, so a test that only awaits it would pass against the broken shape. Only runtime shutdown reveals a detached blocking task. I confirmed the assertion is not vacuous with a throwaway probe: with a live spawn_blocking task, rt.shutdown_timeout(2s) blocks the full 2s, well past the test's 500ms bound.

Verification

Reproduced on main (ba0f96a) against the real deployment: SIGINT during the sign-in wait printed error: interrupted and the process was still alive 10s later. After the fix it exits in 500ms.
End to end against https://fjord.sh: held a stray socket open on the callback port, then hit /callback?code=...&state=.... The stray was dropped at the timeout, the real redirect was accepted, state was checked, the branded success page was served (HTTP 200), and the CLI went on to the token exchange (which correctly failed invalid_grant on the fake code).
cargo fmt --all
cargo clippy --all-targets --all-features -- -D warnings
cargo test: 764 passed, 0 failed, 2 ignored, plus the version integration test.

Not Claimed

The 10 client::resolve::tests were skipped, not run. They read the real keychain, and the test binary's ad-hoc signature needs a fresh keychain grant that could not be presented in my session. They are untouched by this change; CI covers them.
The pre-push hook was bypassed with FJ_SKIP_PREPUSH=1 for the same reason, with the fmt/clippy/test gate run by hand as above.

Fixes #257. ## Fixed and Tested 1. Ctrl+C during `fj auth login` now exits: - `LoopbackServer::wait_for_code` moved off `spawn_blocking` onto `tokio::net::TcpListener`. The accept loop, the request-line read, and the response write are async, so `cli::run`'s SIGINT select actually cancels the wait and the runtime drops with nothing outstanding. - The overall 300s budget moved from a hand-rolled deadline check into a `tokio::time::timeout` around the loop. Same semantics, and it now also covers connection handling. - Covered by `cancelling_wait_for_code_does_not_stall_runtime_shutdown`. 2. A stalled connection no longer wedges the sign-in: - Connections are handled one at a time and the request-line read was unbounded, so a socket that connected and then sent nothing (a browser speculative preconnect, a port scanner) blocked the loop forever and the real redirect behind it was never accepted. - Bounded at `REQUEST_LINE_TIMEOUT` (10s), after which the connection is treated as a stray hit and the loop goes back to accepting. 3. `Cargo.toml`: declared the tokio `net` and `time` features this code now uses directly. Both were already present through feature unification; naming them keeps the build honest if a dependency stops pulling them in. ## On the regression test Asserting on runtime shutdown latency rather than on awaiting the future is deliberate. Awaiting `wait_for_code` looks identical either way, so a test that only awaits it would pass against the broken shape. Only runtime shutdown reveals a detached blocking task. I confirmed the assertion is not vacuous with a throwaway probe: with a live `spawn_blocking` task, `rt.shutdown_timeout(2s)` blocks the full 2s, well past the test's 500ms bound. ## Verification - Reproduced on `main` (ba0f96a) against the real deployment: SIGINT during the sign-in wait printed `error: interrupted` and the process was still alive 10s later. After the fix it exits in 500ms. - End to end against `https://fjord.sh`: held a stray socket open on the callback port, then hit `/callback?code=...&state=...`. The stray was dropped at the timeout, the real redirect was accepted, state was checked, the branded success page was served (HTTP 200), and the CLI went on to the token exchange (which correctly failed `invalid_grant` on the fake code). - `cargo fmt --all` - `cargo clippy --all-targets --all-features -- -D warnings` - `cargo test`: 764 passed, 0 failed, 2 ignored, plus the version integration test. ## Not Claimed - The 10 `client::resolve::tests` were skipped, not run. They read the real keychain, and the test binary's ad-hoc signature needs a fresh keychain grant that could not be presented in my session. They are untouched by this change; CI covers them. - The pre-push hook was bypassed with `FJ_SKIP_PREPUSH=1` for the same reason, with the fmt/clippy/test gate run by hand as above.

stephen added 1 commit

2026-09-06 15:34:42 +00:00

Make the OIDC loopback wait cancellable so Ctrl+C exits

ci / check (pull_request) Successful in 12m22s

Details

ci / live-e2e (pull_request) Successful in 2m19s

Details

ci / coverage (pull_request) Successful in 3m9s

Details

3e3f936a5c

`cli::run` races every command against SIGINT, so Ctrl+C during
`fj auth login` dropped the command future and printed `error:
interrupted` as intended. The process then sat there for up to five
minutes.

`LoopbackServer::wait_for_code` drove its accept loop from
`spawn_blocking`. Dropping the `JoinHandle` only detaches the task, and
dropping the tokio runtime waits for blocking-pool tasks, so the loop
kept polling until `CALLBACK_TIMEOUT` (300s) while the CLI was already
done. The `--device` path was never affected: its polling is ordinary
async, so the select cancels it.

Move the listener to `tokio::net::TcpListener` and make the accept loop,
the request read, and the response write async. Cancellation now
propagates and the runtime drops with nothing outstanding.

Bound the per-connection request-line read at 10s while we're in here.
Connections are handled one at a time and the read was unbounded, so a
socket that connected and sent nothing (a browser speculative
preconnect, a port scanner) blocked the loop forever and the real
redirect queued behind it was never accepted.

The regression test asserts on runtime shutdown latency rather than on
awaiting the future, because awaiting looks identical either way; only
shutdown reveals a detached blocking task.

Closes #257.

stephen merged commit 116be94694 into main

2026-09-06 15:53:43 +00:00

stephen deleted branch fix/oidc-ctrlc-hang

2026-09-06 15:53:43 +00:00

stephen referenced this pull request from a commit

2026-09-06 15:53:44 +00:00

Make the OIDC loopback wait cancellable so Ctrl+C exits (#258)

stephen referenced this pull request

2026-09-06 15:55:57 +00:00

Loopback OIDC timeouts and the callback state check have no test coverage #259

stephen referenced this pull request from a commit

2026-09-06 16:11:57 +00:00