fix+test: guard session-start retry emit, extract + test it, classify raw error

Addresses the #599 review (Stack B):

- B1 (MEDIUM): the `session_start_retry` telemetry emit was unguarded — a
  TaskEvents PutItem fault (throttle/timeout, co-occurring with the transient
  session-start failures this path handles) threw AFTER `autoRetried=true` but
  BEFORE the retry ran, so the user was told a second attempt failed when none
  had, and the real retriable cause was discarded. The emit is now best-effort
  (try/catch → WARN-and-continue) so telemetry never aborts/mis-attributes the
  retry. The `correlation` envelope is threaded so the retry event carries the
  same trace context as `session_started` (#245).

- B2 (crit-9): the auto-retry had ZERO coverage (it lived inline in the durable
  handler's start-session step the suite never invokes). Extracted to an
  exported `startSessionWithRetry(strategy, input, deps)` — matching the repo's
  "test the exported helper" pattern — and unit-tested all four branches
  (success / non-transient no-retry / transient→retry→success /
  transient→transient→throw) plus the B1 best-effort-emit guard.

- N2: classify the RAW error, not a ``Session start failed: …`` wrapper. The
  wrapper string itself matched a TRANSIENT pattern, so EVERY session-start
  failure classified transient and the "non-transient throws immediately" branch
  was effectively dead (a config/auth fault ate a pointless ~1-min retry).
  Classifying the raw string restores that branch (verified: TaskDefinition-
  inactive → retry; ECS_CLUSTER_ARN-not-configured / AccessDenied → surface).

- N1: soften the `[auto-retried]` comment — it forward-referenced
  renderFailureReply/AUTO_RETRIED_MARKER that don't exist yet. Now states the
  marker is persisted verbatim for a forthcoming failure renderer (retryGuidance
  ships ahead of its consumer); no behaviour claim about rendering today.

Full cdk build green (compile + jest + eslint + synth); 5 new helper tests pass.

Author: isadeks

cdk/src/handlers/orchestrate-task.ts

@@ -20,7 +20,6 @@
 import { withDurableExecution, type DurableExecutionHandler } from '@aws/durable-execution-sdk-js';
 import { TaskStatus, TERMINAL_STATUSES } from '../constructs/task-status';
 import { resolveComputeStrategy } from './shared/compute-strategy';
-import { classifyError, isTransientError } from './shared/error-classifier';
 import { reportIssueFailure as reportJiraIssueFailure } from './shared/jira-feedback';
 import { reportIssueFailure } from './shared/linear-feedback';
 import { logger } from './shared/logger';
@@ -38,6 +37,7 @@ import {
   type PollState,
 } from './shared/orchestrator';
 import { runPreflightChecks } from './shared/preflight';
+import { startSessionWithRetry } from './shared/session-start-retry';
 import type { TaskRecord } from './shared/types';
 import { workflowIsReadOnly, workflowRequiresRepo } from './shared/workflows';
 
@@ -169,33 +169,23 @@ const durableHandler: DurableExecutionHandler<OrchestrateTaskEvent, void> = asyn
         payload,
         blueprintConfig,
       };
-      // Transient-error AUTO-RETRY (once). session-start is the ONE place a retry
-      // is idempotent by construction — no repo clone, no commits, no PR have
-      // happened yet, so re-invoking RunTask/InvokeAgentRuntime can't double-run
-      // work. A transient hiccup here (ECS deploy-race "TaskDefinition is inactive",
-      // ENI/capacity delay, a Bedrock/agentcore throttle) usually clears on a second
-      // attempt — so we swallow the first transient failure and try once more before
-      // surfacing anything to the user. A NON-transient failure (bad config, missing
-      // ECS substrate) throws immediately — retrying it just wastes ~a minute. Mid-run
-      // crashes are NOT retried here (step 5); the agent may have pushed commits.
-      let handle;
-      try {
-        handle = await strategy.startSession(startInput);
-      } catch (firstErr) {
-        const classification = classifyError(`Session start failed: ${String(firstErr)}`);
-        if (!isTransientError(classification)) {
-          throw firstErr; // service/user error — a retry won't help; surface now.
-        }
-        autoRetried = true;
-        logger.warn('Session start hit a transient error — auto-retrying once', {
-          task_id: taskId,
-          error: firstErr instanceof Error ? firstErr.message : String(firstErr),
-        });
-        await emitTaskEvent(taskId, 'session_start_retry', {
-          reason: classification?.title ?? 'transient',
-        });
-        handle = await strategy.startSession(startInput);
-      }
+      // Transient-error AUTO-RETRY (once), extracted to startSessionWithRetry so
+      // the four branches are unit-tested (#599 B2). The retry-event emit is
+      // best-effort and guarded there (#599 B1): a TaskEvents PutItem fault can't
+      // abort or mis-attribute the retry. The correlation envelope is threaded so
+      // the session_start_retry event carries the same trace context as
+      // session_started below (#245).
+      const { handle, autoRetried: retried } = await startSessionWithRetry(
+        strategy,
+        startInput,
+        {
+          emitRetryEvent: (reason) =>
+            emitTaskEvent(taskId, 'session_start_retry', { reason }, correlation),
+          logger,
+          taskId,
+        },
+      );
+      autoRetried = retried;
 
       // Build compute metadata for the task record so cancel-task can stop the right backend
       const computeMetadata: Record<string, string> = handle.strategyType === 'ecs'
@@ -221,10 +211,13 @@ const durableHandler: DurableExecutionHandler<OrchestrateTaskEvent, void> = asyn
 
       return handle;
     } catch (err) {
-      // Carry the auto-retry fact into error_message so the channel surface can say
-      // "I already tried again" (a bare marker the classifier ignores but
-      // renderFailureReply detects — see AUTO_RETRIED_MARKER). Only stamped when the
-      // single transient retry above also failed.
+      // Carry the auto-retry fact into error_message: the `[auto-retried]` suffix
+      // is persisted verbatim (the classifier ignores it — it does not affect
+      // classification). It is a breadcrumb for a FORTHCOMING failure renderer to
+      // detect and surface "I already tried again" to the channel — no consumer
+      // renders it yet on this branch (retryGuidance() in error-classifier.ts is
+      // the intended copy source; it ships ahead of its consumer). Only stamped
+      // when the single transient retry above also failed.
       const retriedNote = autoRetried ? ' [auto-retried]' : '';
       await failTask(taskId, TaskStatus.HYDRATING, `Session start failed: ${String(err)}${retriedNote}`, task.user_id, true, task.repo);
       throw err;

cdk/src/handlers/shared/session-start-retry.ts

@@ -0,0 +1,119 @@
+/**
+ *  MIT No Attribution
+ *
+ *  Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ *  Permission is hereby granted, free of charge, to any person obtaining a copy of
+ *  the Software without restriction, including without limitation the rights to
+ *  use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of
+ *  the Software, and to permit persons to whom the Software is furnished to do so.
+ *
+ *  THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ *  IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ *  FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ *  AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ *  LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ *  OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ *  SOFTWARE.
+ */
+
+/**
+ * Session-start transient auto-retry (once), extracted from the durable
+ * ``orchestrate-task`` handler so the four retry branches are unit-testable in
+ * isolation (the handler's inline ``start-session`` step is never invoked by
+ * the test suite). See #599 review B1/B2.
+ *
+ * session-start is the ONE place a retry is idempotent by construction — no repo
+ * clone, no commits, no PR have happened yet, so re-invoking
+ * RunTask/InvokeAgentRuntime can't double-run work. A transient hiccup here (an
+ * ECS deploy-race "TaskDefinition is inactive", ENI/capacity delay, a
+ * Bedrock/agentcore throttle) usually clears on a second attempt, so the first
+ * transient failure is swallowed and retried once. A NON-transient failure (bad
+ * config, missing ECS substrate) is re-thrown immediately — retrying it just
+ * wastes ~a minute. Mid-run crashes are NOT handled here (that's a later step;
+ * the agent may have pushed commits).
+ */
+
+import type { ComputeStrategy, SessionHandle } from './compute-strategy';
+import { classifyError, isTransientError } from './error-classifier';
+
+/** Emit a ``session_start_retry`` telemetry event. Matches the shape of
+ *  ``emitTaskEvent`` bound at the call site (best-effort — see below). */
+export type RetryEventEmitter = (
+  reason: string,
+) => Promise<void>;
+
+/** Minimal logger surface (a subset of the handler's structured logger). */
+export interface RetryLogger {
+  warn(message: string, meta?: Record<string, unknown>): void;
+}
+
+export interface StartSessionWithRetryResult {
+  readonly handle: SessionHandle;
+  /** True iff the first attempt failed transiently and a second attempt ran. */
+  readonly autoRetried: boolean;
+}
+
+/**
+ * Start a compute session, auto-retrying ONCE on a transient failure.
+ *
+ * Behaviour (the four branches #599 B2 asks be covered):
+ *   1. first attempt succeeds            → return it, ``autoRetried: false``.
+ *   2. first attempt fails NON-transient → re-throw the original error (no retry).
+ *   3. first attempt fails transient, retry succeeds → return it, ``autoRetried: true``.
+ *   4. first attempt fails transient, retry also fails → throw the retry's error
+ *      (with ``autoRetried`` observable via the thrown-from state; the caller
+ *      stamps its own ``[auto-retried]`` marker — it owns ``autoRetried`` because
+ *      this function throws rather than returns on the double-failure).
+ *
+ * The ``emitRetryEvent`` call is BEST-EFFORT and internally guarded (B1): a
+ * TaskEvents PutItem fault (throttle/timeout — exactly the conditions that
+ * co-occur with the transient session-start failures this handles) must NOT
+ * abort or mis-attribute the retry. A telemetry failure is logged and swallowed;
+ * the retry proceeds regardless. Previously the emit was unguarded and, if it
+ * threw after ``autoRetried`` was set but before the retry ran, the user was
+ * told a second attempt failed when none had.
+ */
+export async function startSessionWithRetry(
+  strategy: Pick<ComputeStrategy, 'startSession'>,
+  input: Parameters<ComputeStrategy['startSession']>[0],
+  deps: {
+    emitRetryEvent: RetryEventEmitter;
+    logger: RetryLogger;
+    taskId: string;
+  },
+): Promise<StartSessionWithRetryResult> {
+  try {
+    const handle = await strategy.startSession(input);
+    return { handle, autoRetried: false };
+  } catch (firstErr) {
+    // Classify the RAW error, NOT a `Session start failed: …` wrapper (#599 N2):
+    // `/Session start failed/i` is itself a TRANSIENT pattern, so wrapping made
+    // EVERY session-start failure classify transient — a genuine config/auth
+    // fault (missing ECS env, AccessDenied) would eat a pointless ~1-min retry
+    // and the "non-transient throws immediately" branch below was effectively
+    // dead. Classifying the raw string restores that branch. (Widening the
+    // classifier's transient patterns — e.g. ThrottlingException — is a separate
+    // classifier-completeness concern, not this retry gate.)
+    const classification = classifyError(String(firstErr));
+    if (!isTransientError(classification)) {
+      throw firstErr; // service/user error — a retry won't help; surface now.
+    }
+    deps.logger.warn('Session start hit a transient error — auto-retrying once', {
+      task_id: deps.taskId,
+      error: firstErr instanceof Error ? firstErr.message : String(firstErr),
+    });
+    // Best-effort telemetry — a PutItem fault here must never abort the retry
+    // or mis-report the outcome (B1).
+    try {
+      await deps.emitRetryEvent(classification?.title ?? 'transient');
+    } catch (emitErr) {
+      deps.logger.warn('session_start_retry event emit failed (non-fatal)', {
+        task_id: deps.taskId,
+        error: emitErr instanceof Error ? emitErr.message : String(emitErr),
+      });
+    }
+    const handle = await strategy.startSession(input);
+    return { handle, autoRetried: true };
+  }
+}

cdk/test/handlers/shared/session-start-retry.test.ts

@@ -0,0 +1,116 @@
+/**
+ *  MIT No Attribution
+ *
+ *  Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ *  Permission is hereby granted, free of charge, to any person obtaining a copy of
+ *  the Software without restriction, including without limitation the rights to
+ *  use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of
+ *  the Software, and to permit persons to whom the Software is furnished to do so.
+ *
+ *  THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ *  IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ *  FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ *  AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ *  LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ *  OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ *  SOFTWARE.
+ */
+
+import type { SessionHandle } from '../../../src/handlers/shared/compute-strategy';
+import { startSessionWithRetry } from '../../../src/handlers/shared/session-start-retry';
+
+const HANDLE: SessionHandle = {
+  sessionId: 'sess-1',
+  strategyType: 'agentcore',
+  runtimeArn: 'arn:aws:bedrock-agentcore:us-east-1:1:runtime/r',
+};
+
+/** A transient session-start failure the classifier recognizes (ECS deploy race). */
+const TRANSIENT = new Error('TaskDefinition is inactive');
+/** A non-transient failure (a retry won't help). */
+const NON_TRANSIENT = new Error('ECS_CLUSTER_ARN is not configured');
+
+function deps(overrides?: { emitRetryEvent?: (reason: string) => Promise<void> }) {
+  const warns: Array<{ message: string; meta?: Record<string, unknown> }> = [];
+  const emitReasons: string[] = [];
+  return {
+    warns,
+    emitReasons,
+    d: {
+      emitRetryEvent:
+        overrides?.emitRetryEvent ??
+        (async (reason: string) => {
+          emitReasons.push(reason);
+        }),
+      logger: { warn: (message: string, meta?: Record<string, unknown>) => warns.push({ message, meta }) },
+      taskId: 't-1',
+    },
+  };
+}
+
+describe('startSessionWithRetry — the 4 branches (#599 B2)', () => {
+  it('1. first attempt succeeds → returns handle, autoRetried false, no retry', async () => {
+    const startSession = jest.fn().mockResolvedValueOnce(HANDLE);
+    const { d, emitReasons } = deps();
+    const res = await startSessionWithRetry({ startSession }, {} as never, d);
+    expect(res).toEqual({ handle: HANDLE, autoRetried: false });
+    expect(startSession).toHaveBeenCalledTimes(1);
+    expect(emitReasons).toEqual([]); // no retry event on the happy path
+  });
+
+  it('2. first attempt fails NON-transient → re-throws original, NO retry', async () => {
+    const startSession = jest.fn().mockRejectedValueOnce(NON_TRANSIENT);
+    const { d, emitReasons } = deps();
+    await expect(startSessionWithRetry({ startSession }, {} as never, d)).rejects.toBe(NON_TRANSIENT);
+    expect(startSession).toHaveBeenCalledTimes(1); // never retried
+    expect(emitReasons).toEqual([]);
+  });
+
+  it('3. transient then success → returns handle, autoRetried true, emits retry event', async () => {
+    const startSession = jest
+      .fn()
+      .mockRejectedValueOnce(TRANSIENT)
+      .mockResolvedValueOnce(HANDLE);
+    const { d, emitReasons } = deps();
+    const res = await startSessionWithRetry({ startSession }, {} as never, d);
+    expect(res).toEqual({ handle: HANDLE, autoRetried: true });
+    expect(startSession).toHaveBeenCalledTimes(2);
+    expect(emitReasons).toHaveLength(1); // the session_start_retry event fired once
+  });
+
+  it('4. transient then transient → throws the retry error (second failure surfaces)', async () => {
+    const secondErr = new Error('TaskDefinition is inactive (again)');
+    const startSession = jest
+      .fn()
+      .mockRejectedValueOnce(TRANSIENT)
+      .mockRejectedValueOnce(secondErr);
+    const { d } = deps();
+    await expect(startSessionWithRetry({ startSession }, {} as never, d)).rejects.toBe(secondErr);
+    expect(startSession).toHaveBeenCalledTimes(2);
+  });
+});
+
+describe('startSessionWithRetry — retry-event emit is best-effort (#599 B1)', () => {
+  it('a telemetry failure does NOT abort or mis-attribute the retry', async () => {
+    // The exact B1 scenario: the TaskEvents PutItem throws (throttle/timeout,
+    // co-occurring with the transient session-start failure). The retry must
+    // still run and succeed — the emit fault must not surface as the failure.
+    const startSession = jest
+      .fn()
+      .mockRejectedValueOnce(TRANSIENT)
+      .mockResolvedValueOnce(HANDLE);
+    const emitErr = new Error('ProvisionedThroughputExceededException');
+    const { d, warns } = deps({
+      emitRetryEvent: async () => {
+        throw emitErr;
+      },
+    });
+    const res = await startSessionWithRetry({ startSession }, {} as never, d);
+    // Retry proceeded and succeeded despite the emit throwing.
+    expect(res).toEqual({ handle: HANDLE, autoRetried: true });
+    expect(startSession).toHaveBeenCalledTimes(2);
+    // The emit failure was logged (WARN), not propagated.
+    expect(warns.some((w) => w.message.includes('event emit failed'))).toBe(true);
+  });
+});