Skip to content

Commit ce3f6dd

Browse files
jancurnclaude
andauthored
Fix unauthorized sessions to persist without auto-retry (#190)
* Don't auto-retry bearer-token sessions stuck in 'unauthorized' Unauthorized sessions were being flipped back to 'connecting' by consolidateSessions() on every `mcpc` invocation, even when the only credentials were a static bearer token supplied via --header. These retries cannot succeed because the token never changes, so the displayed status kept toggling between 'connecting' and 'unauthorized' and the real state was only surfaced once the user actually hit the session. Scope the auto-retry to sessions with an OAuth profile, where another session may have refreshed the shared tokens in the keychain. Also include the bridge log path in the server auth error so the user has a pointer for debugging, matching how the expired-session error already surfaces the log path. https://claude.ai/code/session_011CjxmErafRX4ypgb2CQkea * Add e2e test for unauthorized bearer session persistence Covers the two guarantees added in the previous commit: - A bearer-only session that fails the initial connect returns exit code 4 and an auth error whose message includes the bridge log path (`check logs at .../bridge-<session>.log`). - The failed session stays `unauthorized` across `mcpc` invocations, even after `lastConnectionAttemptAt` is aged past the 10s auto-retry cooldown. Before the fix, consolidateSessions() would flip the status back to `connecting` whenever the cooldown window had elapsed. The cooldown assertion fakes the post-failure bridge state (old `lastConnectionAttemptAt`, pid cleared) by editing sessions.json directly — otherwise we'd need to wait 10+ seconds for the bridge to exit on its own, which would slow the suite down. Verified both ways: test passes with the fix, reverting only src/lib/sessions.ts causes assertion 3 to fail with exactly the regression message (`expected status to stay 'unauthorized' after cooldown, got: 'connecting'`). https://claude.ai/code/session_011CjxmErafRX4ypgb2CQkea --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 760e277 commit ce3f6dd

6 files changed

Lines changed: 124 additions & 8 deletions

File tree

CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
2020
- `tools-call --task` now prints the task ID and recovery commands when interrupted with Ctrl+C, so you can fetch or cancel the server-side task later
2121
- `tasks-get` no longer suggests `tasks-cancel` when the task has already reached a terminal state (completed, failed, or cancelled)
2222

23+
### Fixed
24+
25+
- Sessions using a static bearer token (via `--header "Authorization: ..."`) no longer flip between `unauthorized` and `connecting` on every `mcpc` invocation — they stay `unauthorized` since retrying the same rejected token cannot succeed without `mcpc login` or reconnecting. OAuth-profile sessions still auto-retry because tokens may have been refreshed by another session
26+
- Server authentication errors now include the path to the bridge log file, so you can inspect it for more detail when investigating why a session was rejected
27+
2328
### Removed
2429

2530
- Removed `tools`, `resources`, and `prompts` shorthand commands — use the full names (`tools-list`, `resources-list`, `prompts-list`) instead

src/cli/commands/sessions.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
validateProfileName,
1212
isProcessAlive,
1313
getServerHost,
14+
getLogsDir,
1415
redactHeaders,
1516
} from '../../lib/index.js';
1617
import { DISCONNECTED_THRESHOLD_MS } from '../../lib/types.js';
@@ -457,7 +458,8 @@ export async function connectSession(
457458
// Fallback: check error message for auth patterns (error may have been wrapped
458459
// as ClientError/ServerError during bridge IPC serialization)
459460
if (detailsError instanceof Error && isAuthenticationError(detailsError.message)) {
460-
throw createServerAuthError(serverConfig.url || target, { sessionName: name });
461+
const logPath = `${getLogsDir()}/bridge-${name}.log`;
462+
throw createServerAuthError(serverConfig.url || target, { sessionName: name, logPath });
461463
}
462464

463465
// Non-auth failure: session was created but server didn't respond properly.

src/lib/bridge-manager.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,10 @@ async function classifyAndThrowSessionError(
7272
logger.warn(`Failed to mark session ${sessionName} as unauthorized:`, e)
7373
);
7474
const target = session.server.url || session.server.command || sessionName;
75+
const logPath = `${getLogsDir()}/bridge-${sessionName}.log`;
7576
throw createServerAuthError(target, {
7677
sessionName,
78+
logPath,
7779
...(originalError && { originalError }),
7880
});
7981
}
@@ -598,7 +600,8 @@ export async function ensureBridgeReady(sessionName: string): Promise<string> {
598600

599601
if (session.status === 'unauthorized') {
600602
const target = session.server.url || session.server.command || sessionName;
601-
throw createServerAuthError(target, { sessionName });
603+
const logPath = `${getLogsDir()}/bridge-${sessionName}.log`;
604+
throw createServerAuthError(target, { sessionName, logPath });
602605
}
603606

604607
if (session.status === 'expired') {

src/lib/errors.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -97,12 +97,11 @@ export function isAuthenticationError(errorMessage: string): boolean {
9797
* Create an AuthError with helpful login guidance for server auth failures
9898
*
9999
* @param target - Server URL or target for login command
100-
* @param options - Optional session name for session-specific guidance
101-
* @param originalError - Original error for debugging
100+
* @param options - Optional session name for session-specific guidance and log path for debugging
102101
*/
103102
export function createServerAuthError(
104103
target: string,
105-
options?: { sessionName?: string; originalError?: Error }
104+
options?: { sessionName?: string; logPath?: string; originalError?: Error }
106105
): AuthError {
107106
let hint: string;
108107
if (options?.sessionName) {
@@ -117,6 +116,10 @@ export function createServerAuthError(
117116
`To authenticate, run:\n` + ` mcpc login ${target}\n\n` + `Then run your command again.`;
118117
}
119118

119+
if (options?.logPath) {
120+
hint += `\n\nFor details, check logs at ${options.logPath}`;
121+
}
122+
120123
return new AuthError(
121124
`Authentication required by server.\n\n` + hint,
122125
options?.originalError ? { originalError: options.originalError } : undefined

src/lib/sessions.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -381,10 +381,15 @@ export async function consolidateSessions(
381381
}
382382

383383
// Identify crashed or unauthorized sessions eligible for automatic reconnection.
384-
// Unauthorized sessions are included because another session sharing the same OAuth
385-
// profile may have refreshed the tokens — the bridge reads from keychain on startup.
384+
// Unauthorized sessions are only included when the session has an OAuth profile —
385+
// another session sharing the same profile may have refreshed the tokens, so a
386+
// retry can succeed as the bridge re-reads from keychain on startup. Sessions
387+
// without a profile (e.g. static bearer token via --header) cannot self-heal, so
388+
// retrying would just flip the status back to 'connecting' on every `mcpc` call
389+
// and hide the real state from the user.
386390
for (const [name, session] of Object.entries(storage.sessions)) {
387-
if ((session?.status === 'crashed' || session?.status === 'unauthorized') && !session.pid) {
391+
const isRetryableUnauthorized = session?.status === 'unauthorized' && !!session.profileName;
392+
if ((session?.status === 'crashed' || isRetryableUnauthorized) && !session.pid) {
388393
// Skip if a connection was already attempted within the cooldown window
389394
const lastAttempt = session.lastConnectionAttemptAt
390395
? new Date(session.lastConnectionAttemptAt).getTime()
Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
#!/bin/bash
2+
# Test: Sessions that fail with 401 and have no OAuth profile stay 'unauthorized'
3+
# across `mcpc` invocations, even after the auto-retry cooldown has elapsed.
4+
# Also verifies the auth error message points at the bridge log file.
5+
6+
source "$(dirname "$0")/../../lib/framework.sh"
7+
test_init "sessions/unauthorized-persist" --isolated
8+
9+
# Start test server requiring auth. The server's auth check accepts any
10+
# "Bearer <anything>" value, so sending an Authorization header that doesn't
11+
# match that shape is enough to trigger 401.
12+
start_test_server REQUIRE_AUTH=true
13+
14+
SESSION=$(session_name "unauth")
15+
_SESSIONS_CREATED+=("$SESSION")
16+
17+
# =============================================================================
18+
# Test: bearer-only session fails with auth error containing log path
19+
# =============================================================================
20+
21+
test_case "connect with bad bearer token fails with auth error + log path"
22+
run_mcpc connect "$TEST_SERVER_URL" "$SESSION" \
23+
--header "X-Test: true" \
24+
--header "Authorization: InvalidScheme not-a-bearer-token"
25+
assert_failure
26+
assert_exit_code 4 "should fail with auth exit code (4)"
27+
assert_contains "$STDERR" "Authentication required by server"
28+
# The error should point at the bridge log file so the user can investigate
29+
assert_contains "$STDERR" "check logs at"
30+
assert_contains "$STDERR" "bridge-${SESSION}.log"
31+
test_pass
32+
33+
# =============================================================================
34+
# Test: session is marked unauthorized right after the failed connect
35+
# =============================================================================
36+
37+
test_case "failed bearer session reports 'unauthorized' status"
38+
run_mcpc --json
39+
assert_success
40+
session_status=$(echo "$STDOUT" | jq -r ".sessions[] | select(.name == \"$SESSION\") | .status")
41+
if [[ "$session_status" != "unauthorized" ]]; then
42+
test_fail "expected status 'unauthorized', got: '$session_status'"
43+
exit 1
44+
fi
45+
test_pass
46+
47+
# =============================================================================
48+
# Test: unauthorized bearer session does NOT flip back to 'connecting'
49+
# after the auto-retry cooldown expires.
50+
#
51+
# Before the fix, consolidateSessions() treated all unauthorized sessions as
52+
# retry candidates and reset the status to 'connecting' on every `mcpc` call
53+
# (after the 10s cooldown), which both hid the real state from the user and
54+
# triggered pointless background bridge restarts. Bearer-only sessions cannot
55+
# self-heal because the token never changes.
56+
# =============================================================================
57+
58+
test_case "unauthorized bearer session stays unauthorized past the cooldown"
59+
sessions_file="$MCPC_HOME_DIR/sessions.json"
60+
assert_file_exists "$sessions_file"
61+
62+
# Age the lastConnectionAttemptAt timestamp well past the 10s cooldown and
63+
# clear the bridge pid to simulate the bridge having exited — that's the real
64+
# state consolidateSessions() sees when the user runs `mcpc` well after the
65+
# original failed connect. Leaving pid set would short-circuit the retry path
66+
# via `!session.pid`, hiding the bug we are trying to catch.
67+
old_iso="2000-01-01T00:00:00.000Z"
68+
tmp_file="$TEST_TMP/sessions.json.$$"
69+
jq --arg name "$SESSION" --arg when "$old_iso" \
70+
'.sessions[$name].lastConnectionAttemptAt = $when
71+
| del(.sessions[$name].pid)' \
72+
"$sessions_file" > "$tmp_file"
73+
mv "$tmp_file" "$sessions_file"
74+
75+
# Re-run the list command — consolidateSessions() runs on every list.
76+
run_mcpc --json
77+
assert_success
78+
session_status=$(echo "$STDOUT" | jq -r ".sessions[] | select(.name == \"$SESSION\") | .status")
79+
if [[ "$session_status" != "unauthorized" ]]; then
80+
test_fail "expected status to stay 'unauthorized' after cooldown, got: '$session_status'"
81+
exit 1
82+
fi
83+
test_pass
84+
85+
# =============================================================================
86+
# Test: using an unauthorized session surfaces the auth error + log path
87+
# =============================================================================
88+
89+
test_case "using unauthorized session surfaces auth error with log path"
90+
run_mcpc "$SESSION" tools-list
91+
assert_failure
92+
assert_exit_code 4 "should fail with auth exit code (4)"
93+
assert_contains "$STDERR" "Authentication required by server"
94+
assert_contains "$STDERR" "check logs at"
95+
assert_contains "$STDERR" "bridge-${SESSION}.log"
96+
test_pass
97+
98+
test_done

0 commit comments

Comments
 (0)