diff --git a/cdk/src/stacks/agent.ts b/cdk/src/stacks/agent.ts index e749ef06..5b5481d7 100644 --- a/cdk/src/stacks/agent.ts +++ b/cdk/src/stacks/agent.ts @@ -612,30 +612,32 @@ export class AgentStack extends Stack { runtimeArnHolder = runtime.agentRuntimeArn; - // --- AgentCore log-delivery: keep the logical ids STABLE across library - // renames, so updating an existing stack never has to be opted into --- + // --- AgentCore log delivery: the library owns these logical ids, not us --- // - // The AgentCore Runtime auto-creates AWS::Logs::DeliverySource + Delivery + - // DeliveryDestination per loggingConfig, naming them from the construct path - // the library happens to use. When that path changes — as it did between - // library versions here — the CFN logical ids change with it, and CFN treats - // renamed resources as new ones: it CREATES before it DELETES. + // The Runtime above creates a DeliverySource / DeliveryDestination / Delivery + // trio per loggingConfig, naming them from its own construct path. We + // deliberately leave those names alone, and that is load-bearing. // - // A DeliverySource is unique per (resource ARN, log type) for the whole - // account, and the runtime ARN does not change across the rename. So the new - // source collides with the live one that is still there, CloudWatch Logs - // rejects it with ``AlreadyExists``, and the whole stack rolls back. Note - // what this means: renaming the resources cannot avoid the collision, because - // the conflict is on the ARN they point at, not on their own names. Only - // keeping the logical id stable avoids it, since that is what makes CFN - // update in place rather than create a second source for the same runtime. + // Renaming any of them is fatal on an existing stack. A DeliverySource is + // unique per (resource ARN, log type) account-wide and the runtime ARN does not + // change, so CloudFormation's create-before-delete produces a second source for + // the same runtime, CloudWatch Logs rejects it as already existing, and the + // whole update rolls back. Note what that implies: no choice of name avoids the + // collision, because the conflict is on the ARN they point at rather than on + // their own names. Only leaving a logical id untouched updates in place. // - // Hence: pinned ALWAYS, for every stack, with no context flag. A flag would - // mean the safe path is the one you have to know to ask for, and the failure - // it prevents is a mid-update rollback that says nothing about the flag's - // existence. A fresh stack is unaffected either way — it has no live sources - // to collide with, and these ids are as valid for it as the library's own. - pinLogDeliveryLogicalIds(runtime); + // An earlier version overrode them from a table of ids recorded off a live + // stack, keyed by stack NAME. Stack name is not a proxy for deployed state: + // two accounts running a stack of the same name had diverged, so the table was + // correct for one and caused the rename on the other. No set of literals can + // describe every account, so we hold none and let the library generate them — + // deterministically, identically in every account, with nothing to keep in sync. + // + // The one cost: a stack deployed before a library-side rename, or held on older + // ids by that table, converges once and needs a one-time operator step first, + // since the old resources must be gone before the new ones are created. That + // step, and how to tell whether a stack needs it, are in + // docs/design/OBSERVABILITY.md ("AgentCore log delivery"). // --- Session storage (preview) --- // The L2 construct does not yet expose filesystemConfigurations; use the @@ -1889,106 +1891,3 @@ export class AgentStack extends Stack { } } -/** - * A churned log-delivery resource to re-pin: the construct child id under the - * Runtime, the logical id CFN already has deployed, and (for the account-unique - * Source/Destination kinds) the deployed ``Name``. ``liveName`` is omitted for - * Delivery links, which have no Name. - */ -interface PinnedLogResource { - readonly childId: string; - readonly liveLogicalId: string; - readonly liveName?: string; -} - -/** - * Log-delivery logical ids to keep stable, keyed by stack name. Consulted on - * every synth — see {@link pinLogDeliveryLogicalIds} for why there is no flag. - * - * Each entry records what CloudFormation already has for a stack deployed before - * the library renamed these resources. Read from `aws cloudformation - * list-stack-resources` against the live stack, so the ids are observed, not - * constructed — the hash in each one is not reproducible from the construct path - * alone, which is precisely why they have to be written down. - * - * An entry stays until its stack is gone. Removing one while the stack still - * exists re-introduces the rename and the failed update that comes with it. - */ -const PINNED_LOG_DELIVERY_BY_STACK: Record = { - 'backgroundagent-dev': [ - { - childId: 'ApplicationLogsDeliverySource', - liveLogicalId: 'RuntimeCDKSourceAPPLICATIONLOGSbackgroundagentdevRuntimeBC0AE9ED96A02E02', - liveName: 'cdk-applicationlogs-source-backgroundagentdevRuntimeBC0AE9ED', - }, - { - childId: 'UsageLogsDeliverySource', - liveLogicalId: 'RuntimeCDKSourceUSAGELOGSbackgroundagentdevRuntimeBC0AE9ED544FBB22', - liveName: 'cdk-usagelogs-source-backgroundagentdevRuntimeBC0AE9ED', - }, - { - childId: 'ApplicationLogsDest', - liveLogicalId: 'RuntimeCdkLogGroupApplicationLogsDeliverybackgroundagentdevRuntimeBC0AE9EDbackgroundagentdevRuntimeApplicationLogGroup454A95E8DestapplicationlogsE09F77DC', - liveName: 'cdk-cwl-Destapplication-logs-dest-backgrounp454A95E829BF8A27', - }, - { - childId: 'UsageLogsDest', - liveLogicalId: 'RuntimeCdkLogGroupUsageLogsDeliverybackgroundagentdevRuntimeBC0AE9EDbackgroundagentdevRuntimeUsageLogGroup7FA1FA67Destusagelogs9AB608D0', - liveName: 'cdk-cwl-Destusage-logs-dest-backgroundagroup7FA1FA67A8A16CEE', - }, - // Delivery links: logical-id pin only (no Name — unique per source/dest pair). - { - childId: 'ApplicationLogsDelivery', - liveLogicalId: 'RuntimeCdkLogGroupApplicationLogsDeliverybackgroundagentdevRuntimeBC0AE9EDbackgroundagentdevRuntimeApplicationLogGroup454A95E8Delivery92FE492C', - }, - { - childId: 'UsageLogsDelivery', - liveLogicalId: 'RuntimeCdkLogGroupUsageLogsDeliverybackgroundagentdevRuntimeBC0AE9EDbackgroundagentdevRuntimeUsageLogGroup7FA1FA67Delivery40F023D7', - }, - ], -}; - -/** - * Pin the auto-created log-delivery resources to stable logical ids, ALWAYS. - * - * These resources are created for us by the AgentCore Runtime and named after - * whatever construct path the library uses internally, so a library-side rename - * silently renames them — and a renamed resource is, to CloudFormation, a new - * one to create before the old is deleted. That is fatal here: a DeliverySource - * is unique per (resource ARN, log type) account-wide, the runtime ARN is - * unchanged by a rename, so the create collides with the live source and the - * update rolls the whole stack back. Owning the ids ourselves decouples us from - * the library's internal naming. - * - * Applied unconditionally rather than behind a flag. Three cases, all safe: - * - * - An existing stack in the account that owns these resources: the ids match - * what CloudFormation already recorded, so it updates them in place. This is - * the case that was broken. - * - A fresh stack or account: nothing owns these names yet, so they create - * normally. The ids are ours rather than the library's, which is the point; - * the values themselves carry no meaning beyond being stable. - * - Any other name: the ids embed the stack name, so each stack gets its own. - * - * The values were read off a stack deployed before the rename. Do not "tidy" - * them — they are a record of what CloudFormation already has, and editing one - * re-breaks exactly the update path this exists to protect. - */ -function pinLogDeliveryLogicalIds(runtime: agentcore.Runtime): void { - const stack = Stack.of(runtime); - const pins = PINNED_LOG_DELIVERY_BY_STACK[stack.stackName]; - // Only the stack these ids were recorded from can use them: they embed that - // stack's name. Any other stack keeps the library's own naming, which is - // correct for it — it has no pre-rename resources to line up with. - if (!pins) return; - - for (const pin of pins) { - const res = runtime.node.tryFindChild(pin.childId) as CfnResource | undefined; - // A future library rename moves the child, so the pin stops matching. Skip - // rather than throw: the stack still deploys, and the next update that hits - // the collision is the signal to re-record the ids from the live stack. - if (!res) continue; - res.overrideLogicalId(pin.liveLogicalId); - if (pin.liveName !== undefined) res.addPropertyOverride('Name', pin.liveName); - } -} diff --git a/cdk/test/stacks/agent.test.ts b/cdk/test/stacks/agent.test.ts index 9d200b0d..d37e3cd7 100644 --- a/cdk/test/stacks/agent.test.ts +++ b/cdk/test/stacks/agent.test.ts @@ -777,59 +777,66 @@ describe('AgentStack', () => { expect(objectStatements).toEqual([]); }); - test('log-delivery logical ids are pinned with NO opt-in, so an existing stack updates in place', () => { - // A DeliverySource is unique per (resource ARN, log type) for the whole - // account, and the runtime ARN survives a library-side rename of these - // auto-created resources. So a renamed source is a SECOND source for the same - // runtime: CloudFormation creates before deleting, CloudWatch Logs rejects it - // as already existing, and the update rolls the whole stack back. + test('no hard-coded log-delivery logical ids or names anywhere in the stack', () => { + // The template must not carry values recorded from one account's live stack. // - // The ids must therefore be pinned unconditionally. Behind a flag, the safe - // path is the one an operator has to already know about, and the failure that - // teaches them is a mid-update rollback whose message never mentions it. + // These resources are created and named by the AgentCore Runtime. An earlier + // version overrode their logical ids from a table keyed by STACK NAME, holding + // ids read off one deployed stack — but stack name is not a proxy for deployed + // state. Two accounts running a stack of the same name had diverged, so the + // table was right for one and actively caused the rename on the other, which is + // fatal: a DeliverySource is unique per (resource ARN, log type) account-wide, + // the runtime ARN survives a rename, so CloudFormation's create-before-delete + // collides with the live source and the whole update rolls back. // - // Asserted on the source, not by synthesizing a second stack: constructing - // one under a different construct id trips an unrelated cdk-nag - // suppression-path check first, which masks whatever this is checking. + // There is no set of literals that serves every account, so the fix is to hold + // none. Asserted on the source because the failure mode is a value being + // reintroduced, not a synth-visible shape. const src = fs.readFileSync( path.resolve(__dirname, '../../src/stacks/agent.ts'), 'utf8', ); - const fn = src.slice(src.indexOf('function pinLogDeliveryLogicalIds')); - const body = fn.slice(0, fn.indexOf('\n}')); - // Keyed off the stack's OWN name — no context, no opt-in. - expect(body).toContain('PINNED_LOG_DELIVERY_BY_STACK[stack.stackName]'); - expect(body).not.toContain('tryGetContext'); - // Nothing anywhere may reintroduce a gate. + // No table, no lookup, no per-stack keying, and no flag to gate any of it. + expect(src).not.toContain('PINNED_LOG_DELIVERY_BY_STACK'); expect(src).not.toContain('pinnedLogDeliveryStack'); + expect(src).not.toContain('overrideLogicalId'); - // The ids it pins are the ones CloudFormation already holds for that stack. - // Hard-coded here on purpose: if someone "tidies" a value in the table, this - // fails instead of the next production update rolling back. - expect(src).toContain('RuntimeCDKSourceAPPLICATIONLOGSbackgroundagentdevRuntimeBC0AE9ED96A02E02'); - expect(src).toContain('RuntimeCDKSourceUSAGELOGSbackgroundagentdevRuntimeBC0AE9ED544FBB22'); + // No captured logical id or account-unique Name, in any of their shapes. + expect(src).not.toMatch(/RuntimeCDKSource/); + expect(src).not.toMatch(/cdk-(application|usage)logs-source-/); + expect(src).not.toMatch(/cdk-cwl-Dest/); }); - test('a stack with no recorded ids keeps the library\'s own log-delivery naming', () => { - // The pinned ids embed a stack name, so they are only correct for that stack. - // Another stack has no pre-rename resources to line up with and must not - // inherit them — otherwise two stacks in one account would claim the same - // account-unique DeliverySource Name. The table lookup is what enforces this, - // so assert it returns nothing for an unknown name rather than falling back. - const src = fs.readFileSync( - path.resolve(__dirname, '../../src/stacks/agent.ts'), 'utf8', - ); - const fn = src.slice(src.indexOf('function pinLogDeliveryLogicalIds')); - const body = fn.slice(0, fn.indexOf('\n}')); - expect(body).toMatch(/if \(!pins\) return;/); - - // And this stack — named TestAgentStack, absent from the table — got the - // library's naming, with none of backgroundagent-dev's ids leaking in. - const ids = Object.keys(template.findResources('AWS::Logs::DeliverySource')); + test('log delivery is left to the library, so every account synthesizes the same ids', () => { + // The point of holding no ids: what lands in the template comes from the + // library, so the same code produces the same ids in every account. A stack + // already on the library's naming sees no change at all. + const sources = template.findResources('AWS::Logs::DeliverySource'); + const ids = Object.keys(sources); expect(ids).toHaveLength(2); + for (const id of ids) { + // Library-generated, not ours: no self-chosen prefix and no stack name baked + // in. A stack name in a logical id is the signature of the old table. expect(id).not.toContain('backgroundagentdev'); + expect(id).not.toContain('AgentRuntimeApplication'); + // Deliberately the library's CURRENT id shape, so this assertion doubles as a + // canary: the next time the AgentCore library renames these resources, this + // fails here rather than in a deploy. Read the failure as an operational + // warning, not a string mismatch — every already-deployed stack will try to + // rename its DeliverySource on the next update, and a renamed source collides + // with the live one on the unchanged runtime ARN and rolls the stack back. + // Update the regex, and add the new shape to the migration note in + // docs/design/OBSERVABILITY.md ("AgentCore log delivery") so operators can + // tell which side of the rename their stack is on. + expect(id).toMatch(/^Runtime(Application|Usage)LogsDeliverySource[0-9A-F]{8}$/); } + + // Both log types are wired, so "no pin" cannot mean "no delivery". + const logTypes = Object.values(sources) + .map((r) => (r as { Properties?: { LogType?: string } }).Properties?.LogType) + .sort(); + expect(logTypes).toEqual(['APPLICATION_LOGS', 'USAGE_LOGS']); }); test('the fan-out consumer can reach BOTH surfaces\' credentials registries', () => { diff --git a/docs/design/OBSERVABILITY.md b/docs/design/OBSERVABILITY.md index b6db7a8d..52824aca 100644 --- a/docs/design/OBSERVABILITY.md +++ b/docs/design/OBSERVABILITY.md @@ -179,6 +179,69 @@ For post-mortems, eval-harness input, and compliance export, the API exposes a s Fields whose source did not run for a given task are returned `null`/empty (e.g. no `--trace` → `trace_uri: null`), so the schema is stable for consumers. +## AgentCore log delivery + +The agent runtime's application and usage logs reach CloudWatch through a trio of resources per log type — `AWS::Logs::DeliverySource`, `AWS::Logs::DeliveryDestination`, and an `AWS::Logs::Delivery` link joining them. The stack does not declare these. The AgentCore `Runtime` L2 construct creates them from the `loggingConfigs` passed to it, and names them from its own internal construct path. + +**The stack deliberately does not override their logical ids.** That is a load-bearing decision rather than an oversight, because the obvious alternatives are worse in a way that is not obvious until a deployment fails. + +### Why a rename of these resources is fatal + +A `DeliverySource` is unique per `(resource ARN, log type)` for the whole account. The agent runtime's ARN does not change when the delivery resources are renamed, so: + +1. Changing a delivery resource's logical id makes CloudFormation treat it as a new resource, which it **creates before deleting** the old one. +2. The new source points at the same runtime ARN as the live one, which still exists at that moment. +3. CloudWatch Logs rejects it — `AlreadyExists`, "This ResourceId has already been used in another Delivery Source in this account." +4. The whole stack update rolls back. + +The counter-intuitive consequence: **no choice of name avoids this.** The conflict is on the ARN the sources point at, not on their own names, so renaming them to library-generated ids, to hand-picked stable ids, or to anything else collides identically. Only leaving a logical id untouched avoids it, because that is what makes CloudFormation update in place instead of creating a second source for the same runtime. + +### Why the ids are not pinned in the template + +An earlier version of the stack held a table of logical ids to override, keyed by stack name, with values read off a live stack. This does not work, because **stack name is not a proxy for deployed state.** Two accounts running a stack of the same name can sit on different library versions' naming, so one set of literals is correct for one account and actively causes the fatal rename on the other. Re-recording the table inverts which account breaks rather than fixing either. + +Since no set of literals can describe every account's deployed state, the stack holds none. Log delivery is left to the library, which generates the ids from the construct path — deterministically, identically in every account, with nothing to keep in sync. + +The cost of this choice is bounded and one-time: a stack deployed before a library-side rename, or held on older ids by the pin table, must converge once. See the migration below. The cost of the alternative was unbounded — a hand-maintained table that silently breaks a different account every time any library version or any deployment moves. + +### Migrating a stack that predates the current naming + +Fresh deployments need nothing here. A stack whose live delivery resources already match what the library generates needs nothing either, and `cdk diff` will show the six resources untouched. + +A stack still on older ids has to converge, and because the create-before-delete collision above applies, the old resources must be gone before the new ones are created. Check first: + +```bash +aws cloudformation list-stack-resources \ + --stack-name backgroundagent-dev \ + --query "StackResourceSummaries[?contains(ResourceType,\ +'Logs::Delivery')].LogicalResourceId" --output text +``` + +Logical ids of the form `RuntimeApplicationLogsDeliverySource` are current — nothing to do. Ids carrying a `CDKSource` segment or the stack name (`RuntimeCDKSourceAPPLICATIONLOGSRuntime`) predate the rename and need the one-time step below. + +Delete the delivery configuration out of band, then deploy. Deliveries first — they reference the source and destination: + +```bash +REGION=us-east-1 +for d in $(aws logs describe-deliveries --region "$REGION" \ + --query 'deliveries[].id' --output text); do + aws logs delete-delivery --region "$REGION" --id "$d" +done +for s in $(aws logs describe-delivery-sources --region "$REGION" \ + --query 'deliverySources[].name' --output text); do + aws logs delete-delivery-source --region "$REGION" --name "$s" +done +for t in $(aws logs describe-delivery-destinations --region "$REGION" \ + --query 'deliveryDestinations[].name' --output text); do + aws logs delete-delivery-destination --region "$REGION" --name "$t" +done +mise //cdk:deploy +``` + +The loops above delete **every** delivery configuration in the account and region, which is what you want on a dedicated deployment account and is not what you want anywhere else. On a shared account, filter to the six resources the previous command listed. + +The deploy then creates the delivery trio under the library's naming and drops the old logical ids, whose underlying resources are already gone — CloudFormation treats a delete of an absent resource as done. Agent logs stop being delivered between the deletion and the end of the deploy; nothing else is affected, and no log data already in CloudWatch is touched. + ## Deployment safety Agent sessions run for up to 8 hours. CDK deployments replace Lambda functions, which can orphan in-flight orchestrator executions. The platform handles this through multiple mechanisms: diff --git a/docs/src/content/docs/architecture/Observability.md b/docs/src/content/docs/architecture/Observability.md index e032fd65..0e50cda7 100644 --- a/docs/src/content/docs/architecture/Observability.md +++ b/docs/src/content/docs/architecture/Observability.md @@ -183,6 +183,69 @@ For post-mortems, eval-harness input, and compliance export, the API exposes a s Fields whose source did not run for a given task are returned `null`/empty (e.g. no `--trace` → `trace_uri: null`), so the schema is stable for consumers. +## AgentCore log delivery + +The agent runtime's application and usage logs reach CloudWatch through a trio of resources per log type — `AWS::Logs::DeliverySource`, `AWS::Logs::DeliveryDestination`, and an `AWS::Logs::Delivery` link joining them. The stack does not declare these. The AgentCore `Runtime` L2 construct creates them from the `loggingConfigs` passed to it, and names them from its own internal construct path. + +**The stack deliberately does not override their logical ids.** That is a load-bearing decision rather than an oversight, because the obvious alternatives are worse in a way that is not obvious until a deployment fails. + +### Why a rename of these resources is fatal + +A `DeliverySource` is unique per `(resource ARN, log type)` for the whole account. The agent runtime's ARN does not change when the delivery resources are renamed, so: + +1. Changing a delivery resource's logical id makes CloudFormation treat it as a new resource, which it **creates before deleting** the old one. +2. The new source points at the same runtime ARN as the live one, which still exists at that moment. +3. CloudWatch Logs rejects it — `AlreadyExists`, "This ResourceId has already been used in another Delivery Source in this account." +4. The whole stack update rolls back. + +The counter-intuitive consequence: **no choice of name avoids this.** The conflict is on the ARN the sources point at, not on their own names, so renaming them to library-generated ids, to hand-picked stable ids, or to anything else collides identically. Only leaving a logical id untouched avoids it, because that is what makes CloudFormation update in place instead of creating a second source for the same runtime. + +### Why the ids are not pinned in the template + +An earlier version of the stack held a table of logical ids to override, keyed by stack name, with values read off a live stack. This does not work, because **stack name is not a proxy for deployed state.** Two accounts running a stack of the same name can sit on different library versions' naming, so one set of literals is correct for one account and actively causes the fatal rename on the other. Re-recording the table inverts which account breaks rather than fixing either. + +Since no set of literals can describe every account's deployed state, the stack holds none. Log delivery is left to the library, which generates the ids from the construct path — deterministically, identically in every account, with nothing to keep in sync. + +The cost of this choice is bounded and one-time: a stack deployed before a library-side rename, or held on older ids by the pin table, must converge once. See the migration below. The cost of the alternative was unbounded — a hand-maintained table that silently breaks a different account every time any library version or any deployment moves. + +### Migrating a stack that predates the current naming + +Fresh deployments need nothing here. A stack whose live delivery resources already match what the library generates needs nothing either, and `cdk diff` will show the six resources untouched. + +A stack still on older ids has to converge, and because the create-before-delete collision above applies, the old resources must be gone before the new ones are created. Check first: + +```bash +aws cloudformation list-stack-resources \ + --stack-name backgroundagent-dev \ + --query "StackResourceSummaries[?contains(ResourceType,\ +'Logs::Delivery')].LogicalResourceId" --output text +``` + +Logical ids of the form `RuntimeApplicationLogsDeliverySource` are current — nothing to do. Ids carrying a `CDKSource` segment or the stack name (`RuntimeCDKSourceAPPLICATIONLOGSRuntime`) predate the rename and need the one-time step below. + +Delete the delivery configuration out of band, then deploy. Deliveries first — they reference the source and destination: + +```bash +REGION=us-east-1 +for d in $(aws logs describe-deliveries --region "$REGION" \ + --query 'deliveries[].id' --output text); do + aws logs delete-delivery --region "$REGION" --id "$d" +done +for s in $(aws logs describe-delivery-sources --region "$REGION" \ + --query 'deliverySources[].name' --output text); do + aws logs delete-delivery-source --region "$REGION" --name "$s" +done +for t in $(aws logs describe-delivery-destinations --region "$REGION" \ + --query 'deliveryDestinations[].name' --output text); do + aws logs delete-delivery-destination --region "$REGION" --name "$t" +done +mise //cdk:deploy +``` + +The loops above delete **every** delivery configuration in the account and region, which is what you want on a dedicated deployment account and is not what you want anywhere else. On a shared account, filter to the six resources the previous command listed. + +The deploy then creates the delivery trio under the library's naming and drops the old logical ids, whose underlying resources are already gone — CloudFormation treats a delete of an absent resource as done. Agent logs stop being delivered between the deletion and the end of the deploy; nothing else is affected, and no log data already in CloudWatch is touched. + ## Deployment safety Agent sessions run for up to 8 hours. CDK deployments replace Lambda functions, which can orphan in-flight orchestrator executions. The platform handles this through multiple mechanisms: