From 8ed2b884952d46151bc8690d329a34d3cd3b5905 Mon Sep 17 00:00:00 2001 From: dtoro Date: Fri, 10 Jul 2026 00:44:51 +0200 Subject: [PATCH] fix: pct_create false-success, VMID collision, and stuck approval banner Real production failure when the operator clicked Approve in chat: nothing provisioned, banner never cleared, execution marked completed. Three root causes: - sshExec swallowed non-zero exits when the command produced output, so a `pct create` that printed "CT 132 already exists" and failed was reported as success and a bogus lxc entity was registered. Now any non-zero exit returns an error (with output) so the execution is correctly marked failed. - The LLM reused VMID 132 (belongs to lxc:rclone; VMIDs are cluster-wide). pct_create now checks in-use VMIDs via `pvesh get /cluster/resources` and falls back to `pvesh get /cluster/nextid` when the requested id is taken. - InlineApproval.svelte reset its state on every prop change (done was also compared against the wrong string), so the banner never cleared and each click re-POSTed /decision. Rewritten to track outcome per executionId, clear on success, and block resubmits. Co-Authored-By: Claude Opus 4.8 --- internal/httpapi/phase3.go | 42 ++++++++++++- web/src/lib/components/InlineApproval.svelte | 64 +++++++++++--------- 2 files changed, 75 insertions(+), 31 deletions(-) diff --git a/internal/httpapi/phase3.go b/internal/httpapi/phase3.go index fec290c..ccbdca8 100644 --- a/internal/httpapi/phase3.go +++ b/internal/httpapi/phase3.go @@ -9,6 +9,7 @@ import ( "fmt" "log/slog" "os" + "strconv" "strings" "time" @@ -103,10 +104,18 @@ func sshExec(ctx context.Context, host, user, command string) (string, error) { defer session.Close() out, err := session.CombinedOutput(command) - if err != nil && out == nil { - return "", fmt.Errorf("exec: %w", err) + text := strings.TrimSpace(string(out)) + // A non-zero exit MUST surface as an error. The previous guard only + // errored when there was no output, so a `pct create` that printed + // "CT 132 already exists" and exited non-zero was reported as success — + // the execution was marked completed though nothing was provisioned. + if err != nil { + if text != "" { + return text, fmt.Errorf("%w: %s", err, text) + } + return text, fmt.Errorf("exec: %w", err) } - return strings.TrimSpace(string(out)), nil + return text, nil } func resolveHostSSH(ctx context.Context, pool *db.Pool, entitySlug string) (string, string, error) { @@ -276,6 +285,33 @@ func executeApprovedAction(ctx context.Context, pool *db.Pool, execID uuid.UUID, return } + // VMID collision guard. Proxmox VMIDs are cluster-wide, so the model's + // guess (e.g. 132) can collide with a container on another node — pct + // create then fails with "CT N already exists on node X". Fetch the set + // of in-use VMIDs across the cluster; if the requested id is taken (or + // absent), fall back to the cluster's next free id so provisioning + // still succeeds instead of dead-ending on the operator's approval. + usedRaw, _ := sshExec(ctx, host, user, `pvesh get /cluster/resources --type vm --output-format json 2>/dev/null | grep -o '"vmid":[0-9]*' | grep -o '[0-9]*' || true`) + used := map[int]bool{} + for _, l := range strings.Fields(usedRaw) { + if n, e := strconv.Atoi(strings.TrimSpace(l)); e == nil { + used[n] = true + } + } + if cfg.VMID == 0 || used[cfg.VMID] { + nextRaw, nerr := sshExec(ctx, host, user, `pvesh get /cluster/nextid 2>/dev/null`) + nextID, cerr := strconv.Atoi(strings.TrimSpace(nextRaw)) + if nerr != nil || cerr != nil || nextID == 0 { + msg := fmt.Sprintf("VMID %d is already in use on the cluster and could not resolve a free id", cfg.VMID) + pool.Exec(ctx, `UPDATE executions SET status='failed', result=$2::jsonb WHERE entity_id=$1`, + execID, fmt.Sprintf(`{"error":%q}`, msg)) + emitExecutionEvent(ctx, pool, execID, "failed", map[string]any{"target": targetSlug, "error": msg}) + return + } + slog.Info("httpapi: pct_create VMID reassigned", "requested", cfg.VMID, "assigned", nextID) + cfg.VMID = nextID + } + privFlag := "--unprivileged 1" if cfg.Privileged { privFlag = "--unprivileged 0" diff --git a/web/src/lib/components/InlineApproval.svelte b/web/src/lib/components/InlineApproval.svelte index d750d6c..3618d13 100644 --- a/web/src/lib/components/InlineApproval.svelte +++ b/web/src/lib/components/InlineApproval.svelte @@ -2,6 +2,7 @@ import type { PendingApproval } from '$lib/stores/chat' import { decideApproval } from '$lib/api' import { Button } from '$lib/components/ui/button' + import { SvelteMap } from 'svelte/reactivity' import CheckIcon from '@lucide/svelte/icons/check' import XIcon from '@lucide/svelte/icons/x' import ShieldCheckIcon from '@lucide/svelte/icons/shield-check' @@ -9,34 +10,51 @@ let { approvals }: { approvals: PendingApproval[] } = $props() - let pending = $state(false) - let done = $state(null) - let doneId = $state('') + // Per-execution outcome, keyed by executionId. A resolved entry hides the + // action buttons for that approval permanently so the banner clears after a + // click and can never re-POST /decision. 'pending' guards against double + // submits (the old component reset its state on every prop change, so the + // banner never cleared and each click fired another decision). + type State = 'pending' | 'approved' | 'denied' | 'failed' + const outcome = new SvelteMap() async function decide(approval: PendingApproval, decision: 'approve' | 'deny') { - pending = true - doneId = approval.executionId - const result = await decideApproval(approval.executionId, decision) - pending = false - done = result ? decision : 'failed' + const id = approval.executionId + const cur = outcome.get(id) + if (cur === 'pending' || cur === 'approved' || cur === 'denied') return // no resubmit + outcome.set(id, 'pending') + const ok = await decideApproval(id, decision) + outcome.set(id, ok ? (decision === 'approve' ? 'approved' : 'denied') : 'failed') } - - $effect(() => { - done = null - doneId = '' - pending = false - void approvals - }) -{#each approvals.filter(a => !done || a.executionId !== doneId) as approval (approval.executionId)} - {#if !done || approval.executionId !== doneId} +{#each approvals as approval (approval.executionId)} + {@const state = outcome.get(approval.executionId)} + {#if state === 'approved'} +
+ + Approved — provisioning. Track progress in the Executions view. +
+ {:else if state === 'denied'} +
+ + Denied. +
+ {:else if state === 'failed'} +
+ + Decision failed to send. + +
+ {:else}
{approval.action} on {approval.target} requires approval - {#if pending && doneId === approval.executionId} + {#if state === 'pending'} {:else} {/if}
- {:else if done === 'approved'} -
- - Approved. The action is running. -
- {:else} -
- - Denied. -
{/if} {/each}