From 0945e21eb1540f5babc1fc56a083a419ba33d1d3 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Tue, 30 Jun 2026 14:59:48 -0700 Subject: [PATCH] Address second review pass: fix start-time race + minor cleanups - M1 (race): don't mount the host AWS dir for static-credential Bedrock. sync_bedrock_credentials() is the sole writer of ~/.aws/credentials in that mode, and mounting /tmp/.host-aws let the entrypoint's `rm -rf ~/.aws; cp -a` race that write at startup (only when a global aws_config_path was also set). Static keys + AWS_REGION env are self-sufficient and don't need the host config, so skipping the mount removes the dual-writer entirely. - L-a: exit codes are now read via wait_for_exec_exit(), which polls inspect_exec until the exec reports finished, so a non-zero tar/cred exit isn't missed by reading exit_code too early. The backup only fails on a definitively non-zero code (falls back to the empty-output check if undeterminable). - L-b: fixed two comments that referenced the old write_bedrock_static_credentials name (now sync_bedrock_credentials). - L-c: entrypoint only rewrites ~/.claude.json when awsAuthRefresh is actually present, avoiding a needless jq reformat on every non-SSO start. - L-d: backup script traps EXIT to remove its mktemp staging dir even when tar fails, so failed backups don't accumulate temp dirs (with the sanitized config copy) in the container. L-e (drop routing) is a non-issue: the layout is tabbed, so only one terminal pane is ever visible; the active-guard routing is correct. Verified the race fix, trap cleanup, grep guard, and exit-code polling. cargo check / tsc / vitest all pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- app/src-tauri/src/commands/file_commands.rs | 18 ++++++------ app/src-tauri/src/docker/container.rs | 18 ++++++++++-- app/src-tauri/src/docker/exec.rs | 32 +++++++++++++++------ container/entrypoint.sh | 4 ++- 4 files changed, 50 insertions(+), 22 deletions(-) diff --git a/app/src-tauri/src/commands/file_commands.rs b/app/src-tauri/src/commands/file_commands.rs index 85a5ae5..9e48f20 100644 --- a/app/src-tauri/src/commands/file_commands.rs +++ b/app/src-tauri/src/commands/file_commands.rs @@ -206,6 +206,7 @@ pub async fn download_container_backup( // so secrets can't leak through the sanitization fallback. let script = r#"set -e STAGE=$(mktemp -d) +trap 'rm -rf "$STAGE"' EXIT mkdir -p "$STAGE/home-claude" if [ -f "$HOME/.claude.json" ]; then if ! jq 'del(.primaryApiKey, .oauthAccount, .customApiKeyResponses)' "$HOME/.claude.json" \ @@ -221,8 +222,7 @@ fi tar czf - --ignore-failed-read \ --exclude='*/node_modules' --exclude='*/target' \ -C "$TC_BACKUP_SRC" . \ - -C "$STAGE" home-claude -rm -rf "$STAGE""#; + -C "$STAGE" home-claude"#; let cmd = vec!["sh".to_string(), "-c".to_string(), script.to_string()]; @@ -291,17 +291,15 @@ rm -rf "$STAGE""#; // The tar pipeline can abort mid-stream (producing a truncated archive) and // still have sent bytes, so a non-zero exit must be treated as failure even - // when `total > 0`. - let exit_code = docker - .inspect_exec(&exec.id) - .await - .map(|i| i.exit_code.unwrap_or(0)) - .unwrap_or(0); + // when `total > 0`. Poll until the exec actually reports finished so the + // exit code is reliably populated; if it can't be determined we fall back to + // the `total == 0` check below. + let exit_code = crate::docker::exec::wait_for_exec_exit(&exec.id).await; - if stream_err.is_none() && exit_code != 0 { + if stream_err.is_none() && exit_code.is_some_and(|c| c != 0) { stream_err = Some(format!( "Backup command failed (exit {}){}", - exit_code, + exit_code.unwrap_or(-1), if stderr_text.trim().is_empty() { String::new() } else { diff --git a/app/src-tauri/src/docker/container.rs b/app/src-tauri/src/docker/container.rs index 19a840a..0c9f997 100644 --- a/app/src-tauri/src/docker/container.rs +++ b/app/src-tauri/src/docker/container.rs @@ -278,7 +278,7 @@ fn compute_bedrock_fingerprint(project: &Project, global_aws: &GlobalAwsSettings // NOTE: the static credential fields (access key / secret / session // token) are intentionally NOT part of the fingerprint. They are // written to ~/.aws/credentials on every start by - // write_bedrock_static_credentials(), so a key rotation should refresh + // sync_bedrock_credentials(), so a key rotation should refresh // in place rather than force a full container recreation. Region, // profile, and bearer token remain env-based and so stay here. let parts = vec![ @@ -674,7 +674,7 @@ pub async fn create_container( BedrockAuthMethod::StaticCredentials => { // Static/session credentials are NOT injected as env vars. // They are written to ~/.aws/credentials by - // write_bedrock_static_credentials() on every container + // sync_bedrock_credentials() on every container // start, so rotated/updated keys are picked up without a // full container recreation (and never get baked into the // snapshot image). The empty values set by the @@ -934,7 +934,19 @@ pub async fn create_container( false }; - if should_mount_aws || aws_config_path.is_some() { + // For static-credential Bedrock, sync_bedrock_credentials() is the sole + // owner of ~/.aws/credentials (it rewrites it on every start). Mounting the + // host AWS dir would make the entrypoint's `rm -rf ~/.aws; cp -a` race that + // write at startup, so we never mount it in that case — the static keys + // (+ AWS_REGION env) are self-sufficient and don't need the host config. + let is_bedrock_static = project.backend == Backend::Bedrock + && project + .bedrock_config + .as_ref() + .map(|b| b.auth_method == BedrockAuthMethod::StaticCredentials) + .unwrap_or(false); + + if (should_mount_aws || aws_config_path.is_some()) && !is_bedrock_static { let aws_dir = aws_config_path .map(|p| std::path::PathBuf::from(p)) .or_else(|| dirs::home_dir().map(|h| h.join(".aws"))); diff --git a/app/src-tauri/src/docker/exec.rs b/app/src-tauri/src/docker/exec.rs index 41d0e21..2cba63a 100644 --- a/app/src-tauri/src/docker/exec.rs +++ b/app/src-tauri/src/docker/exec.rs @@ -344,14 +344,30 @@ pub async fn exec_oneshot_env_status( StartExecResults::Detached => return Err("Exec started in detached mode".to_string()), } - // Exit code is only available after the process has finished (the stream above - // has drained), so inspect now. - let exit_code = docker - .inspect_exec(&exec.id) - .await - .map_err(|e| format!("Failed to inspect exec: {}", e))? - .exit_code - .unwrap_or(0); + // The output stream draining doesn't strictly guarantee inspect_exec has the + // final exit_code populated yet, so poll until the exec reports finished. + let exit_code = wait_for_exec_exit(&exec.id).await.unwrap_or(0); Ok((combined, exit_code)) } + +/// Poll `inspect_exec` until the exec reports finished and return its exit code. +/// Returns `None` if the code can't be determined (inspect error, or the exec +/// doesn't report finished within ~1s — which shouldn't happen once its output +/// stream has drained). +pub async fn wait_for_exec_exit(exec_id: &str) -> Option { + let docker = get_docker().ok()?; + for _ in 0..40 { + match docker.inspect_exec(exec_id).await { + Ok(info) => { + if info.running != Some(true) { + // Finished: use the reported code (default 0 if somehow absent). + return Some(info.exit_code.unwrap_or(0)); + } + } + Err(_) => return None, + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + None +} diff --git a/container/entrypoint.sh b/container/entrypoint.sh index 320a186..4bcf08f 100644 --- a/container/entrypoint.sh +++ b/container/entrypoint.sh @@ -231,7 +231,9 @@ if [ -n "$AWS_SSO_AUTH_REFRESH_CMD" ]; then chown claude:claude "$CLAUDE_JSON" chmod 600 "$CLAUDE_JSON" unset AWS_SSO_AUTH_REFRESH_CMD -elif [ -f "$CLAUDE_JSON" ]; then +elif [ -f "$CLAUDE_JSON" ] && grep -q '"awsAuthRefresh"' "$CLAUDE_JSON" 2>/dev/null; then + # Only rewrite when the key is actually present, to avoid a needless jq + # reformat of ~/.claude.json on every start of a non-SSO backend. MERGED=$(jq 'del(.awsAuthRefresh)' "$CLAUDE_JSON" 2>/dev/null) if [ -n "$MERGED" ]; then printf '%s\n' "$MERGED" > "$CLAUDE_JSON"