diff --git a/docs/docs/self-host/deploy/01-deploy-remotely.mdx b/docs/docs/self-host/deploy/01-deploy-remotely.mdx index 0e9ee2f3e7..79015be5c0 100644 --- a/docs/docs/self-host/deploy/01-deploy-remotely.mdx +++ b/docs/docs/self-host/deploy/01-deploy-remotely.mdx @@ -230,6 +230,36 @@ traefik: Recreate the API and Traefik containers afterwards. +### Agent Runs Fail With `record log is unreadable; cannot rebuild the conversation` + +This applies if the deployment answers on a public hostname and agent runs fail while the rest of +the app works. Other symptoms of the same cause: an agent that forgets earlier turns, or runner logs +showing `cred=DROPPED(endpoint-not-agenta-ingest)` and `HTTP 401` on `/sessions/records/ingest`, +`/sessions/records/query`, and `/sessions/streams/heartbeat`. + +**Cause:** The `runner` service has no `AGENTA_API_URL`. Each run carries the trace endpoint the API +built from its public base, for example `https://agenta.example.com/api/otlp/v1/traces`, while the +runner knows only its internal hop, `http://api:8000`. The runner cannot tell that endpoint apart +from a third-party OTLP collector, so it withholds the run's credential and every callback to the +API is rejected. Conversation history is never written and never read back. + +**Solution:** Set `AGENTA_API_URL` to the same public API base the `api` and `services` containers +use, and recreate the runner: + +```bash +docker compose up -d --force-recreate runner +``` + +Compose reads `AGENTA_API_URL` from the shell (or `--env-file`) when it builds the runner's +environment, not from a service's `env_file`. Setting it only in a file that is passed as `env_file` +reaches the API and services containers but not the runner. Confirm it landed: + +```bash +docker compose exec runner printenv AGENTA_API_URL +``` + +Sessions that ran while this was broken keep the gaps in their history. New sessions are unaffected. + ### Nginx-Specific Issues If you chose the Nginx deployment option: diff --git a/docs/docs/self-host/reference/01-configuration.mdx b/docs/docs/self-host/reference/01-configuration.mdx index 3bffe7d683..4060aab2f1 100644 --- a/docs/docs/self-host/reference/01-configuration.mdx +++ b/docs/docs/self-host/reference/01-configuration.mdx @@ -446,17 +446,32 @@ caller's idle timeout on the streaming transport. ### Callback API -Set on the `runner` service. It gives the runner the in-network address of the API for session -heartbeats, working-directory mount signing, and the trace-export fallback. +Set both on the `runner` service. The runner calls the API back for session heartbeats, +conversation persistence, working-directory mount signing, and the trace-export fallback. | Variable | Role | Default | Helm | |---|---|---|---| -| `AGENTA_API_INTERNAL_URL` | API locator for runner callbacks | Compose: `http://api:8000` | Wired by the chart | - -Point this at the API as reached from inside the runner container (its Compose or cluster service -name, for example `http://api:8000`), not the public URL, which does not resolve inside a -container. If it is unset, the runner falls back to the public `AGENTA_API_URL` and then to the -base inferred from each request. +| `AGENTA_API_INTERNAL_URL` | Where the runner sends its callbacks | Compose: `http://api:8000` | Wired by the chart | +| `AGENTA_API_URL` | Which trace endpoints the runner recognizes as this deployment | Unset | Wired by the chart | + +The two answer different questions, and setting one does not cover the other. + +`AGENTA_API_INTERNAL_URL` is an address. Point it at the API as reached from inside the runner +container (its Compose or cluster service name, for example `http://api:8000`), not the public URL, +which does not resolve inside a container. If it is unset, the runner falls back to `AGENTA_API_URL` +and then to the base inferred from each request. + +`AGENTA_API_URL` is an identity. Every run arrives carrying the trace endpoint the API built from +its own public base, for example `https://agenta.example.com/api/otlp/v1/traces`. The runner uses +that endpoint to decide whether the run's credential belongs to this platform or to a third-party +OTLP collector the caller aimed the run at, and it forwards the credential to platform calls only in +the first case. It cannot make that call from the internal address alone, because the internal hop +never appears in a dispatched run. + +Set `AGENTA_API_URL` to the same public API base the `api` and `services` containers use. When it is +unset, the runner logs a warning at startup, treats every run's credential as belonging to this +platform, and logs a second warning naming the endpoint it could not attribute. Runs keep working; +credentials aimed at a third-party collector are no longer kept out of platform calls. ### Internal settings diff --git a/hosting/docker-compose/oss/env.oss.dev.example b/hosting/docker-compose/oss/env.oss.dev.example index 2a3b2cfff7..6f65ca6c91 100644 --- a/hosting/docker-compose/oss/env.oss.dev.example +++ b/hosting/docker-compose/oss/env.oss.dev.example @@ -18,6 +18,10 @@ AGENTA_LICENSE=oss # ================================================================== # AGENTA_WEB_URL=http://localhost AGENTA_SERVICES_URL=http://localhost/services +# The runner reads this too, to recognize which trace endpoints are this deployment and so +# whether a run's credential may authenticate its callbacks. Compose reads it from the shell +# (or --env-file), not from a service's env_file, so it must be exported when you bring the +# stack up. AGENTA_API_URL=http://localhost/api # AGENTA_API_INTERNAL_URL=http://api:8000 diff --git a/hosting/docker-compose/oss/env.oss.gh.example b/hosting/docker-compose/oss/env.oss.gh.example index 895374b698..e1ab99123e 100644 --- a/hosting/docker-compose/oss/env.oss.gh.example +++ b/hosting/docker-compose/oss/env.oss.gh.example @@ -22,6 +22,11 @@ AGENTA_LICENSE=oss # ================================================================== # AGENTA_WEB_URL=http://localhost AGENTA_SERVICES_URL=http://localhost/services +# The runner reads this too, to recognize which trace endpoints are this deployment and so +# whether a run's credential may authenticate its callbacks. Compose reads it from the shell +# (or --env-file), not from a service's env_file, so it must be exported when you bring the +# stack up. Without it the runner warns at startup and stops filtering third-party +# collector credentials out of platform calls. AGENTA_API_URL=http://localhost/api # AGENTA_API_INTERNAL_URL=http://api:8000 diff --git a/services/runner/src/engines/sandbox_agent/runtime-policy.ts b/services/runner/src/engines/sandbox_agent/runtime-policy.ts index 56f335e8ee..03761637df 100644 --- a/services/runner/src/engines/sandbox_agent/runtime-policy.ts +++ b/services/runner/src/engines/sandbox_agent/runtime-policy.ts @@ -1,8 +1,10 @@ import { type AgentRunRequest, type ToolPermission } from "../../protocol.ts"; import { claimSessionOwnership, REPLICA_ID } from "../../sessions/alive.ts"; import { + configuredIngestBases, isAgentaIngest, platformAuthorizationProvider, + publicApiBaseConfigured, resolveOtlpTraceEndpoint, type AuthorizationProvider, } from "../../tracing/otel.ts"; @@ -19,16 +21,65 @@ export function runCredential(request: AgentRunRequest): string { return (headers["authorization"] ?? headers["Authorization"] ?? "").trim(); } +/** Endpoints already warned about, so a per-turn read warns once instead of every run. */ +const warnedEndpoints = new Set(); + +/** Test-only: forget which endpoints have warned, so a case can assert on its own warning. */ +export function resetPlatformCredentialWarnings(): void { + warnedEndpoints.clear(); +} + /** * The legacy wire has one authorization header for two possible owners. Treat it as an Agenta * platform credential only when the configured destination is Agenta ingest; for an external * collector it belongs exclusively to that collector and must never enter platform calls. + * + * That attribution is only decidable once the runner knows its platform's PUBLIC api base + * (`AGENTA_API_URL`), because the public form is what a dispatched run carries: the API hands the + * SDK `https:///api`, while the runner's own hop is usually the internal `http://api:8000`. + * A runner told ONLY its internal hop cannot tell its own API under its public name from a + * third-party collector. Refusing there fails closed on the wrong axis — it silently strips the + * credential from every run in an otherwise healthy self-hosted deployment, and the damage + * surfaces far away as a 401 on session persistence. So the strict check arms itself only when + * the operator has supplied the base that makes it decidable, and otherwise keeps the credential + * and says loudly what to configure. */ -export function platformCredentialForRequest(request: AgentRunRequest): string { +export function platformCredentialForRequest( + request: AgentRunRequest, + log: Log = (message) => process.stderr.write(`${message}\n`), +): string { const endpoint = resolveOtlpTraceEndpoint( request.telemetry?.exporters?.otlp?.endpoint, ); - return isAgentaIngest(endpoint) ? runCredential(request) : ""; + if (isAgentaIngest(endpoint)) return runCredential(request); + + const credential = runCredential(request); + if (!credential) return ""; + + if (!publicApiBaseConfigured()) { + if (!warnedEndpoints.has(endpoint)) { + warnedEndpoints.add(endpoint); + log( + `[sessions] WARNING: trace endpoint ${endpoint} matches no configured Agenta ingest ` + + `base (${configuredIngestBases().join(", ")}), and AGENTA_API_URL is not set, so the ` + + `run credential cannot be attributed. Using it for platform calls anyway. Set ` + + `AGENTA_API_URL to this deployment's public api base (e.g. https:///api) to ` + + `attribute it properly and to keep third-party collector credentials out of platform calls.`, + ); + } + return credential; + } + + if (!warnedEndpoints.has(endpoint)) { + warnedEndpoints.add(endpoint); + log( + `[sessions] trace endpoint ${endpoint} is not Agenta ingest ` + + `(${configuredIngestBases().join(", ")}); dropping the run credential from platform ` + + `calls. Session persistence and history rebuild will fail with HTTP 401 if this ` + + `endpoint IS this deployment's api base.`, + ); + } + return ""; } export interface RunOtlpTarget { diff --git a/services/runner/src/server.ts b/services/runner/src/server.ts index 692dc04949..fe94e2573e 100644 --- a/services/runner/src/server.ts +++ b/services/runner/src/server.ts @@ -58,7 +58,11 @@ import { type KeepaliveConfig, type KeepaliveProviderName, } from "./engines/sandbox_agent/session-identity.ts"; -import { platformCredentialForRequest } from "./engines/sandbox_agent/runtime-policy.ts"; +import { + platformCredentialForRequest, + runCredential, +} from "./engines/sandbox_agent/runtime-policy.ts"; +import { publicApiBaseConfigured } from "./tracing/otel.ts"; import { SessionPool } from "./engines/sandbox_agent/session-pool.ts"; import { runnerInfo } from "./version.ts"; import { subscriptionStatusResponse } from "./subscription-status.ts"; @@ -407,10 +411,17 @@ async function runAndStreamWithApiBaseResolved( // append, interaction rows) must see the SAME execution id the alive-lock and records use. request.turnId = turnId; - // Diagnostic: surface whether the session-owned persist/alive path is entered and - // whether the invoke credential arrived. Empty cred => heartbeat/persist would 401. + // Diagnostic: surface whether the session-owned persist/alive path is entered and whether the + // invoke credential arrived. Empty cred => heartbeat/persist would 401. The two empty cases have + // different fixes, so name them apart: ABSENT means the caller sent no credential, DROPPED means + // one arrived but did not attribute to this platform (see `platformCredentialForRequest`). + const credentialState = platformCredentialForRequest(request) + ? "present" + : runCredential(request) + ? "DROPPED(endpoint-not-agenta-ingest)" + : "ABSENT(caller-sent-none)"; process.stderr.write( - `[sessions] stream sessionOwned=${sessionOwned} sessionId=${sessionId ?? "-"} turnId=${turnId ?? "-"} cred=${platformCredentialForRequest(request) ? "present" : "MISSING"}\n`, + `[sessions] stream sessionOwned=${sessionOwned} sessionId=${sessionId ?? "-"} turnId=${turnId ?? "-"} cred=${credentialState}\n`, ); // Session-owned runs survive client disconnect — the runner owns the run. Non-session @@ -884,6 +895,16 @@ if (isEntrypoint(import.meta.url)) { process.stderr.write( `[sandbox-agent] http server listening on ${runnerConfig.server.host}:${runnerConfig.server.port}\n`, ); + if (!publicApiBaseConfigured()) { + process.stderr.write( + "[sandbox-agent] WARNING: AGENTA_API_URL is not set. Dispatched runs carry this " + + "deployment's PUBLIC api base in their trace endpoint, so without it the runner " + + "cannot tell its own api from a third-party collector and cannot attribute the run " + + "credential. Set AGENTA_API_URL to the public api base (e.g. https:///api); " + + `AGENTA_API_INTERNAL_URL (${process.env.AGENTA_API_INTERNAL_URL ?? "unset"}) is the ` + + "in-network hop and does not substitute for it.\n", + ); + } if (insecureEgressAllowed()) { process.stderr.write( "[sandbox-agent] WARNING: AGENTA_INSECURE_EGRESS_ALLOWED is set: user MCPs may " + diff --git a/services/runner/src/tracing/otel.ts b/services/runner/src/tracing/otel.ts index e6daa0ab33..dc5ccca54d 100644 --- a/services/runner/src/tracing/otel.ts +++ b/services/runner/src/tracing/otel.ts @@ -352,15 +352,35 @@ export function isAgentaIngest(endpoint: string): boolean { const normalizedEndpoint = normalize(endpoint); if (!normalizedEndpoint) return false; + return configuredIngestBases().some( + (base) => + normalize(`${base.replace(/\/+$/, "")}/otlp/v1/traces`) === + normalizedEndpoint, + ); +} + +/** The api bases `isAgentaIngest` accepts, in precedence order. Exported so a rejection can name + * what it compared against — the failure is always a configuration gap, never a code path. */ +export function configuredIngestBases(): string[] { return [ process.env.AGENTA_API_INTERNAL_URL, process.env.AGENTA_API_URL, CLOUD_API_BASE, - ].some( - (base) => - base && - normalize(`${base.replace(/\/+$/, "")}/otlp/v1/traces`) === - normalizedEndpoint, + ].filter((base): base is string => Boolean(base)); +} + +/** + * Has the operator told this runner its platform's PUBLIC api base? + * + * Only `AGENTA_API_URL` counts. `AGENTA_API_INTERNAL_URL` is the in-network hop and never appears + * in a dispatched run's trace endpoint, so it cannot settle whether a public-looking endpoint is + * this platform or someone else's collector. Cloud is self-describing: a runner reaching the cloud + * api needs no operator input, so the built-in cloud base counts as configured. + */ +export function publicApiBaseConfigured(): boolean { + return Boolean( + process.env.AGENTA_API_URL?.trim() || + process.env.AGENTA_API_INTERNAL_URL?.trim()?.startsWith(CLOUD_API_BASE), ); } diff --git a/services/runner/tests/unit/platform-credential-attribution.test.ts b/services/runner/tests/unit/platform-credential-attribution.test.ts new file mode 100644 index 0000000000..5c488f14f5 --- /dev/null +++ b/services/runner/tests/unit/platform-credential-attribution.test.ts @@ -0,0 +1,164 @@ +/** + * `platformCredentialForRequest` decides whether the run's Authorization header may authenticate + * the runner's calls back to the Agenta api (heartbeat, record ingest, record query). One header + * on the wire has two possible owners: this platform, or a third-party OTLP collector the caller + * aimed the run at. Getting that wrong in the permissive direction leaks a collector's token into + * platform calls; getting it wrong in the strict direction silently strips the credential and every + * session call 401s, which surfaces far away as "record log is unreadable". + * + * The attribution is only decidable when the runner knows its platform's PUBLIC api base, because + * that is the form a dispatched run carries (`https:///api/otlp/v1/traces`) while the + * runner's own hop is usually internal (`http://api:8000`). These cases pin both directions, + * including the self-hosted shape that regressed: internal hop configured, public base not. + * + * Run: pnpm exec vitest run tests/unit/platform-credential-attribution.test.ts + */ +import assert from "node:assert/strict"; +import { afterEach, beforeEach, describe, it, vi } from "vitest"; + +import type { AgentRunRequest } from "../../src/protocol.ts"; +import { + platformCredentialForRequest, + resetPlatformCredentialWarnings, +} from "../../src/engines/sandbox_agent/runtime-policy.ts"; +import { publicApiBaseConfigured } from "../../src/tracing/otel.ts"; + +const PUBLIC_BASE = "https://selfhosted.example.com/api"; +const PUBLIC_ENDPOINT = `${PUBLIC_BASE}/otlp/v1/traces`; +const INTERNAL_BASE = "http://api:8000"; +const CREDENTIAL = "Secret run-credential"; + +function request( + endpoint: string, + authorization = CREDENTIAL, +): AgentRunRequest { + return { + telemetry: { + exporters: { + otlp: { + endpoint, + headers: authorization ? { authorization } : {}, + }, + }, + }, + } as unknown as AgentRunRequest; +} + +/** Collect the warnings a call emits instead of writing them to the suite's stderr. */ +function withLog(): { lines: string[]; log: (message: string) => void } { + const lines: string[] = []; + return { lines, log: (message) => lines.push(message) }; +} + +beforeEach(() => { + resetPlatformCredentialWarnings(); + // Neither var is scrubbed by the hermetic-env setup, and a loaded dev env sets both — which + // would flip exactly the case this file is about. Start from "operator configured nothing". + vi.stubEnv("AGENTA_API_URL", undefined); + vi.stubEnv("AGENTA_API_INTERNAL_URL", undefined); +}); + +afterEach(() => { + vi.unstubAllEnvs(); + resetPlatformCredentialWarnings(); +}); + +describe("platform credential attribution", () => { + it("uses the credential when the endpoint matches the configured public base", () => { + vi.stubEnv("AGENTA_API_URL", PUBLIC_BASE); + const { lines, log } = withLog(); + + assert.equal( + platformCredentialForRequest(request(PUBLIC_ENDPOINT), log), + CREDENTIAL, + ); + assert.deepEqual(lines, []); + }); + + it("uses the credential when the endpoint matches the internal hop", () => { + vi.stubEnv("AGENTA_API_INTERNAL_URL", INTERNAL_BASE); + const { lines, log } = withLog(); + + assert.equal( + platformCredentialForRequest( + request(`${INTERNAL_BASE}/otlp/v1/traces`), + log, + ), + CREDENTIAL, + ); + assert.deepEqual(lines, []); + }); + + it("keeps the credential when only the internal hop is configured, and says what to set", () => { + // The self-hosted shape that regressed in v0.114.0: the runner knows `http://api:8000`, the + // dispatched run carries the public base, and the two never string-match. Refusing here strips + // the credential from a correctly deployed platform, so the runner keeps it and names the gap. + vi.stubEnv("AGENTA_API_INTERNAL_URL", INTERNAL_BASE); + const { lines, log } = withLog(); + + assert.equal( + platformCredentialForRequest(request(PUBLIC_ENDPOINT), log), + CREDENTIAL, + ); + assert.equal(lines.length, 1); + assert.match(lines[0]!, /AGENTA_API_URL is not set/); + assert.match(lines[0]!, /selfhosted\.example\.com/); + }); + + it("drops the credential for a foreign endpoint once the public base IS configured", () => { + // With the public base known, a non-matching endpoint really is someone else's collector, + // so the third-party protection the strict check was written for stays armed. + vi.stubEnv("AGENTA_API_URL", PUBLIC_BASE); + vi.stubEnv("AGENTA_API_INTERNAL_URL", INTERNAL_BASE); + const { lines, log } = withLog(); + + assert.equal( + platformCredentialForRequest( + request("https://collector.thirdparty.example/v1/traces"), + log, + ), + "", + ); + assert.equal(lines.length, 1); + assert.match(lines[0]!, /dropping the run credential/); + }); + + it("returns empty without warning when the caller sent no credential", () => { + vi.stubEnv("AGENTA_API_INTERNAL_URL", INTERNAL_BASE); + const { lines, log } = withLog(); + + assert.equal( + platformCredentialForRequest(request(PUBLIC_ENDPOINT, ""), log), + "", + ); + assert.deepEqual(lines, []); + }); + + it("warns once per endpoint, not once per turn", () => { + vi.stubEnv("AGENTA_API_INTERNAL_URL", INTERNAL_BASE); + const { lines, log } = withLog(); + + for (let turn = 0; turn < 3; turn += 1) { + platformCredentialForRequest(request(PUBLIC_ENDPOINT), log); + } + + assert.equal(lines.length, 1); + }); +}); + +describe("publicApiBaseConfigured", () => { + it("is false when only the internal hop is set", () => { + vi.stubEnv("AGENTA_API_INTERNAL_URL", INTERNAL_BASE); + assert.equal(publicApiBaseConfigured(), false); + }); + + it("is true when the public base is set", () => { + vi.stubEnv("AGENTA_API_URL", PUBLIC_BASE); + assert.equal(publicApiBaseConfigured(), true); + }); + + it("is true for a runner pointed straight at cloud, which needs no operator input", () => { + vi.stubEnv("AGENTA_API_INTERNAL_URL", "https://cloud.agenta.ai/api"); + assert.equal(publicApiBaseConfigured(), true); + }); +}); diff --git a/services/runner/tests/unit/server.test.ts b/services/runner/tests/unit/server.test.ts index 5f4b019217..ebe2ccb96b 100644 --- a/services/runner/tests/unit/server.test.ts +++ b/services/runner/tests/unit/server.test.ts @@ -653,6 +653,11 @@ describe("createAgentServer", () => { }); it("never sends a third-party collector credential to session APIs", async () => { + // The protection is only decidable once the runner knows its own PUBLIC api base: without it + // a public-looking endpoint could equally be this platform under its public name. Configure it + // here so this case exercises the armed check rather than the undecidable state (which the + // sibling case below covers). + vi.stubEnv("AGENTA_API_URL", "https://agenta.example.test/api"); let engineCredential: string | undefined; const s = await listen(async (_request, _emit, _signal, options) => { engineCredential = options?.credential?.(); @@ -720,6 +725,71 @@ describe("createAgentServer", () => { } }); + it("still authenticates session APIs when only the internal api hop is configured", async () => { + // The self-hosted shape that broke in v0.114.0. The api and services containers know the + // deployment by its public name, so a dispatched run's trace endpoint is the public base, + // while the runner is given only the in-network hop. Those two never string-match, and + // dropping the credential there 401s every session call — persistence, heartbeat, and the + // history rebuild that reports it as "record log is unreadable". Undecidable must not mean + // unauthenticated. + vi.stubEnv("AGENTA_API_URL", undefined); + vi.stubEnv("AGENTA_API_INTERNAL_URL", "http://api:8000"); + let engineCredential: string | undefined; + const s = await listen(async (_request, _emit, _signal, options) => { + engineCredential = options?.credential?.(); + return { ok: true, output: "done", events: [] }; + }); + const realFetch = globalThis.fetch.bind(globalThis); + const platformCalls: Array<{ url: string; authorization: string }> = []; + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockImplementation(async (input, init) => { + const url = String(input); + if (url === `${s.url}/run`) return realFetch(input, init); + const headers = (init?.headers ?? {}) as Record; + platformCalls.push({ url, authorization: headers.authorization ?? "" }); + if (url.endsWith("/sessions/streams/heartbeat")) { + return Response.json({ + stream: { id: "stream-1" }, + is_current_turn: true, + }); + } + return Response.json({}); + }); + + try { + const res = await fetchSpy(`${s.url}/run`, { + method: "POST", + headers: { accept: "application/x-ndjson", ...AUTH }, + body: JSON.stringify({ + harness: "pi_core", + sessionId: "session-self-hosted", + telemetry: { + exporters: { + otlp: { + endpoint: "https://selfhosted.example.test/api/otlp/v1/traces", + headers: { authorization: "Secret platform-credential" }, + }, + }, + }, + messages: [{ role: "user", content: "hello" }], + }), + }); + await res.text(); + + assert.equal(engineCredential, "Secret platform-credential"); + assert.equal( + platformCalls.some( + (call) => call.authorization === "Secret platform-credential", + ), + true, + ); + } finally { + fetchSpy.mockRestore(); + await s.close(); + } + }); + it("persists a legacy image-only tail without attachment references", async () => { let runCalls = 0; const s = await listen(async () => {