aws-samples / aws-samples/sample-agent-assisted-sdlc
fix: Setup Lambda has no retry on transient AgentCore HTTP errors — single failure aborts whole pipeline
- Dominant language
- Python
- Stars
- 42
- Forks
- 9
- PR merge metrics
- No merged PRs in 30d
Description
## Description
The Setup Lambda has zero retry on `execute_command` calls into the AgentCore runtime. A single transient HTTP failure from the AgentCore commands API aborts the entire pipeline run — the Step Functions execution fails, no error comment is posted to the issue (the failure happens before the orchestrator's exception handler runs), and the user sees only that the `agent:start` label is still set with no progress.
This bit issue #33 on its first re-trigger (2026-06-04 20:54 UTC, Step Functions execution `issue-33-1780606445`). Sequence:
1. `clone_repo_done` OK
2. `plugin_mount_check` OK
3. `plugin_copy_result` OK
4. `_write_file_chunked(issue.json)` succeeded for chunks 1-4 (each `contentStop exitCode=0 status=COMPLETED`)
5. `_write_file_chunked(issue.json)` chunk 5 — **`requests.RequestException` from the AgentCore commands API** → `RuntimeCommandError: AgentCore execute_command HTTP failure` at `pipeline.py:79`, propagated up through `base.py:80` (`_write_file_chunked`) and `base.py:138` (`setup_workspace`) → unhandled at `index.py:153`
6. Step Functions execution FAILED, ~67 seconds wall-clock, no comment posted
7. Re-toggling `agent:start` succeeded on the next attempt (transient)
Same issue.json was 24 KB on disk (chunked write was mid-stream when chunk 5 failed). The runtime had a partial workspace: cloned repo, plugins copied, issue.json truncated, no `invocation-1/`, no `current/` symlink — i.e., the pipeline got nowhere usable, but the cleanup wasn't catastrophic because `/mnt/workplace/` is persistent NFS and a re-run overwrites.
The transient AgentCore HTTP error is not deterministic and not under our control. The right fix is **defensive retry inside the Setup Lambda**, not chasing the underlying transport.
## Scope decision locked in (read before implementing)
**Retry ONLY on `requests.ConnectionError`. Do NOT retry on `Timeout` or 5xx.**
This is non-negotiable and the reasoning is a correctness constraint, not a preference — see the Idempotency section below. A `ConnectionError` is raised before the TCP/TLS connection is established or before the request body is sent, so we know **the command never reached the runtime** and replaying it is safe. A `Timeout` or a 5xx, by contrast, can occur *after* the runtime already executed the command but before/while the response is delivered — replaying those can double-execute the command, which corrupts `issue.json` (the `>>`-append chunked write) and any other non-idempotent call site. Restricting to `ConnectionError` is the only policy that is safe for every current caller of `execute_command` without adding per-call-site idempotency machinery.
(If a future need for broader retry on `Timeout`/5xx arises, it requires the "Option B" idempotency rework described in a follow-up — write each chunk to a `.tmp` file then `mv` atomically so a replay overwrites rather than appends. That is explicitly OUT OF SCOPE here.)
## Out of scope — a SEPARATE AgentCore failure mode
There is a second, distinct AgentCore commands API failure that this issue does **not** address: the API returns HTTP 200 with a well-formed EventStream containing `contentStart` + `contentStop` (`exitCode=0, status=COMPLETED`) but **no `contentDelta` event carrying stdout**. The command succeeds but its stdout is dropped. This surfaces as the `no_output_captured` WARNING at `pipeline.py:122` and, when the caller relies on stdout, as a downstream crash — e.g. issue #33 on 2026-06-04 03:43 UTC, where `git rev-parse --abbrev-ref HEAD` returned empty stdout and `refresh_for_reinvocation` raised `Invalid branch name: ''`.
That failure mode does **not** raise `requests.RequestException`, so the retry loop in THIS issue would never fire for it. PR #40 (`.git` probe in `refresh_for_reinvocation`) partially mitigates the specific branch-read case by failing fast with a clear `WorkspaceSetupError`, but the general "successful command, dropped stdout" problem is unsolved. Do NOT try to solve it here. If you want to address it, it needs its own retry-on-empty-stdout-when-exit-0 logic at the EventStream-parsing layer (lines 85-135), which is a different change with different idempotency implications. Flag it as a follow-up in the PR body; do not expand this PR's scope to cover it.
## Acceptance Criteria
- [ ] `project-management/shared/pipeline.py::execute_command` gains a bounded retry loop around the `requests.post` + `raise_for_status()` block. Retry **only** on `requests.ConnectionError`, up to 3 attempts total (1 initial + 2 retries) with 1s then 2s sleeps between attempts. After exhausting retries, raise `RuntimeCommandError` as today.
- [ ] **Do NOT retry on any other exception.** `requests.Timeout`, `requests.HTTPError` (any status, including 5xx and 429), and every other `requests.RequestException` subclass must raise `RuntimeCommandError` immediately, with no retry. (Rationale: only `ConnectionError` guarantees the command did not reach the runtime — see the Idempotency section.)
- [ ] Each retry attempt re-signs the request via `sign_request(...)` before re-issuing. SigV4 signatures are time-bounded; reusing the original signed headers across a multi-second backoff can hit `RequestExpired`. Build a fresh `signed_headers` inside the loop each attempt.
- [ ] `logger.warning` on each retry attempt with `extra={"attempt": N, "error_type": "ConnectionError"}`. `logger.exception` (or the existing raise path) on final failure as today. Do not log the request body (it may contain base64 issue content).
- [ ] No retry on `stop_runtime_session` — caller already wraps in try/except and continues, and one failure is fine. Add a one-line comment in `pipeline.py::stop_runtime_session` explaining why retry is intentionally absent there.
- [ ] No retry on `_write_file_chunked` itself — retry happens inside `execute_command` per-chunk. If a chunk's `execute_command` exhausts all 3 attempts and still raises, the whole chunked write aborts with `RuntimeCommandError` (this is the existing behavior; do not change it).
- [ ] New unit tests in `project-management/shared/tests/test_pipeline.py` (new file if not present). **Mock `time.sleep`** so the backoff does not actually delay the suite. Cover:
- Single transient `ConnectionError` then success → retries once, succeeds, `time.sleep` called once.
- 3 consecutive `ConnectionError` → exhausts retries, raises `RuntimeCommandError`, `time.sleep` called twice.
- `requests.Timeout` → raises `RuntimeCommandError` immediately, NO retry, `time.sleep` NOT called.
- `requests.HTTPError` with a 503 response → raises immediately, NO retry (verifies 5xx is NOT retried under the locked-in ConnectionError-only policy).
- `requests.HTTPError` with a 400 response → raises immediately, NO retry.
- Each retry attempt calls `sign_request` again (assert call count on a mock — 1 call on success-first-try, N calls when retried N-1 times).
- [ ] All existing tests still pass (`npm test`, `pytest`, `bash test/hooks/test_hooks.sh`, `npx cdk synth --quiet`, `ruff check`).
- [ ] PR uses the template from `.github/pull_request_template.md`.
## Files to Modify
- `project-management/shared/pipeline.py` — add the ConnectionError-only retry loop in `execute_command` around the `requests.post` block; re-sign each attempt; add the explanatory comment in `stop_runtime_session`.
- `project-management/shared/tests/test_pipeline.py` — new file (or extend existing). Mock `requests.post`, `sign_request`/`botocore` SigV4, and `time.sleep`.
## Cross-Resource Interactions
- **`coding-assistants/claude-code/runtime/`** — the FastAPI runtime is the OTHER side of the AgentCore commands API call. With ConnectionError-only retry, the runtime never sees a replayed command, because a `ConnectionError` means the request never left the client cleanly. So there is no double-execution risk at the runtime for any call site, idempotent or not.
- **`project-management/github/connector/lambda/index.py`** — handler at line 153 catches nothing today (the `RuntimeCommandError` propagates as a Step Functions task failure). Don't change this — Step Functions fails the run, which is the correct behavior after retries are exhausted. We do not want the Setup Lambda swallowing errors that should fail the run.
- **Step Functions state machine** — the Setup state currently has no retry policy on `RuntimeCommandError`. Out of scope for this PR (CDK change in a different file). A Step-Functions-level retry would re-run the WHOLE Setup Lambda (re-clone, re-write), which is a coarser and more expensive retry than the per-command retry this PR adds; the two are complementary but the SFN one is a separate issue if wanted.
- **Lifecycle phases the change must survive:**
- **create** — n/a (Python-only change in shared lib).
- **update** — `cdk deploy` updates the Lambda code package; existing in-flight invocations are not retried. n/a.
- **create-rollback** — n/a.
- **update-rollback** — n/a.
- **destroy** — n/a.
- **Idempotency (the reason for ConnectionError-only).** Several `execute_command` call sites are NOT safe to replay if the command already executed:
- `_write_file_chunked` writes chunk 1 with `>` (truncate) and chunks 2..N with `>>` (append). If chunk 5's request reached the runtime and executed (appended 6 KB), but the response was lost to a `Timeout` or 5xx, replaying chunk 5 appends the same 6 KB a SECOND time. The result is a corrupted `issue.json` with a duplicated slice — invalid JSON that Test 6 (chunked-write verification) would only catch after the fact. This is exactly why retrying `Timeout`/5xx is unsafe and why the policy is ConnectionError-only: `ConnectionError` is the one class where the runtime provably never received the command.
- The invocation-rotation command (`mkdir invocation-$N && ln -sfn ...`) and `git` mutation commands have similar replay hazards.
- Restricting to `ConnectionError` sidesteps all of this without per-call-site idempotency guards. The broader-retry alternative (Option B: `.tmp`-write-then-`mv`) is a larger change deliberately left for a follow-up.
- **Error codes** — under the ConnectionError-only policy, HTTP status codes do NOT drive retry decisions: any `HTTPError` (4xx or 5xx) raises immediately. The retry trigger is the transport-layer `ConnectionError`, which has no HTTP status. State this explicitly in the PR body so a reviewer doesn't expect 5xx-based retry.
## Constraints
- Do NOT replace `requests` with `urllib3.Retry` or boto3's built-in retry. The retry policy here must be explicit about retrying ONLY `ConnectionError` and re-signing each attempt; both alternatives obscure those decisions and tend to retry on 5xx by default, which is unsafe here.
- Do NOT add new third-party dependencies. The retry is a simple `for attempt in range(3): try: ... except requests.ConnectionError: sleep; continue` loop. `tenacity` / `backoff` are overkill and add a runtime dep the Lambda doesn't have today.
- Do NOT retry on `stop_runtime_session`. Caller already tolerates failure.
- Do NOT retry on `Timeout` or 5xx. (Repeated here because it's the most likely thing to get wrong.)
- Do NOT change `RuntimeCommandError`'s message or signature. Existing callers branch on the exception type, not the message.
- Do NOT use `git add -A` when committing.
- All Python files keep the Apache-2.0 header.
## Verification Beyond Tests
- [ ] `npx cdk synth --quiet` passes (no template change expected — Lambda code change only).
- [ ] `cd project-management/shared && python3 -m pytest tests/ -v` passes; new test cases cover the six retry scenarios from AC; `time.sleep` is mocked (suite does not slow down).
- [ ] `bash test/hooks/test_hooks.sh` passes (unaffected).
- [ ] `ruff check project-management coding-assistants gateway source-control lib` passes.
- [ ] Manual verification (post-deploy):
- Trigger a normal pipeline run on a small test issue. CloudWatch logs for the Setup Lambda should show NO `attempt: N` retry warnings on the happy path.
- The transient `ConnectionError` cannot be reproduced on demand. Document this gap in the PR body — manual verification is happy-path only; the retry path is covered by unit tests, not a live repro.
- [ ] PR body must include a `## Known Interactions` section explaining (a) why ConnectionError-only is a correctness constraint (the `>>`-append double-execution corruption), (b) that `Timeout`/5xx are intentionally NOT retried and what the safe broader-retry path (Option B) would require, (c) that the separate empty-stdout-on-success failure mode is explicitly out of scope and why the retry loop doesn't address it, (d) the happy-path-only manual-verification gap.
## CI Requirements
```bash
npm test # TypeScript tests
cd project-management/shared && python3 -m pytest tests/ -v # Python tests
bash test/hooks/test_hooks.sh # Hook tests
npx cdk synth --quiet # CDK validation
ruff check . # Python lint
```
All Python files must include the Apache-2.0 license header. Read `CLAUDE.md` for full conventions and security rules. When opening the PR, read and follow `.github/pull_request_template.md`.
## References
- The bug instance: Step Functions execution `arn:aws:states:us-west-2:407296935140:execution:agent-assisted-sdlc-pipeline_sdlc_pipeline:issue-33-1780606445` (FAILED 2026-06-04 20:55 UTC).
- Setup Lambda log group: `/aws/lambda/agent-assisted-sdlc-pipeline-a-SetupLambdaECFA7C8A-t1Yl785xnJoC`. RequestId `8dfbb0d5-62cf-41a5-a404-1576c3e9f5df` shows the full crash trace.
- The relevant code paths:
- `project-management/shared/pipeline.py:72-79` — the un-retried `requests.post` + `RuntimeCommandError` raise site (this is what gains the retry loop).
- `project-management/shared/pipeline.py:122-129` — the `no_output_captured` warning for the SEPARATE empty-stdout failure mode (out of scope here).
- `project-management/shared/assistants/base.py:80` — `_write_file_chunked` calls `execute_command` per chunk (the `>>`-append idempotency hazard).
- `project-management/shared/assistants/base.py:138` — `setup_workspace` calls `_write_file_chunked` for `issue.json`.
- `project-management/github/connector/lambda/index.py:153` — Setup Lambda handler entry point that propagates `RuntimeCommandError` to Step Functions.
- Related but distinct: issue #34 (orchestrator stale-base + hallucinated-merge) — different layer (orchestrator skill / re-invocation), not the Setup Lambda. **Sequence #34 before this issue** so the two PRs (both eventually touching the re-invocation path) don't race; this PR touches `pipeline.py`, #34 touches `base.py` + skill files, so there's no file conflict, but landing #34 first keeps the re-invocation behavior settled.
- Related but distinct: PR #40 (`.git` probe) — partially mitigates the empty-stdout failure mode for the branch-read case only.
- Related but distinct: PRs #30 / #31 / #32 — runtime hardening. The transport itself is fine; this issue is about Setup Lambda resilience to the transient `ConnectionError` from AgentCore.
Contributor guide
Research direction
Start in project-management/shared/pipeline.py at execute_command and stop_runtime_session, then read or create project-management/shared/tests/test_pipeline.py. Run the mocked retry tests and the existing pytest suite; done means only ConnectionError is retried with fresh signing and backoff, other request failures are immediate, stop_runtime_session remains unchanged, and the listed verification commands pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- aws, python
- Domain
- backend, cloud
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 76/100