Skip to content

fix(testing): reject blank execution ARNs - #636

Merged
yaythomas merged 2 commits into
aws:mainfrom
SravyaPeri:fix/461-blank-execution-arn-validation
Aug 13, 2026
Merged

fix(testing): reject blank execution ARNs#636
yaythomas merged 2 commits into
aws:mainfrom
SravyaPeri:fix/461-blank-execution-arn-validation

Conversation

@SravyaPeri

Copy link
Copy Markdown
Contributor

Issue #, if available:: #461

Description of changes:

This PR adds execution ARN validation to the in-memory Executor used by the local test runner. Two things were missing:

  • No input sanitation: a None/empty/whitespace-only execution_arn was never rejected before reaching store/registry internals.
  • get_execution_state, stop_execution, and checkpoint_execution all call self._registry.submit(execution_arn, ...), which calls get_or_create(), so a bad ARN silently registered a permanent phantom worker (with its own thread/lane) before the existence check inside the task body ever ran and raised ResourceNotFoundException.

Note on scope: this runner's execution ARNs are opaque local identifiers (Execution.new() generates f"{uuid4()}/{invocation_id}"), not real AWS ARNs — confirmed by checking that neither the runtime code nor the existing test fixtures (e.g. executor_test.py) use the real arn:aws:lambda:...:function:.../durable-execution/.../... shape. So this
validates blank/non-string input rather than checking AWS ARN structure, which would have rejected legitimate executions.

Changes:

  • Executor._require_valid_arn(): raises InvalidParameterValueException for a blank/non-string ARN
  • Wired into get_execution, stop_execution, get_execution_state, checkpoint_execution, before registry.submit() in the latter three, closing the phantom-worker leak
  • 12 new tests (4 methods x 3 blank-input variants), including assertions that registry.active_count() == 0 after a rejected call

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@SravyaPeri
SravyaPeri force-pushed the fix/461-blank-execution-arn-validation branch from bcbdfde to 9632f07 Compare August 13, 2026 12:55
@SravyaPeri
SravyaPeri deployed to ai-pr-review August 13, 2026 12:55 — with GitHub Actions Active
@SravyaPeri
SravyaPeri temporarily deployed to ai-pr-review-runtime August 13, 2026 15:45 — with GitHub Actions Inactive
@SravyaPeri
SravyaPeri temporarily deployed to ai-pr-review-runtime August 13, 2026 15:45 — with GitHub Actions Inactive
@github-actions

Copy link
Copy Markdown
Contributor

Codex AI review

No actionable findings. Residual risk: whitespace-only ARNs are covered at the Executor boundary, but not through the HTTP routing layer.

Reviewed commit 9632f071f4c014990057051782c5a23f9205c75c. Workflow run

@github-actions

Copy link
Copy Markdown
Contributor

Claude AI review

No actionable findings.

This PR adds Executor._require_valid_arn() and calls it in get_execution, stop_execution, get_execution_state, and checkpoint_execution to reject blank/None/non-string ARNs with InvalidParameterValueException.

I verified:

  • Phantom-worker fix is correct. stop_execution/get_execution_state/checkpoint_execution route through self._registry.submit(...)get_or_create(), which creates a persistent ExecutionWorker + lane thread that is only removed on a COMPLETED teardown. A bad ARN previously leaked a worker before the in-task existence check raised ResourceNotFoundException. Placing the guard before submit closes this, and the tests assert _registry.active_count() == 0 against the real (non-mocked) registry.
  • Coverage is consistent. Guarding the shared get_execution also transitively protects get_execution_details and get_execution_history. All other registry-submitting paths (callbacks, timeouts, re-invoke) derive the ARN from parsed tokens/internal state and are never externally blank.
  • No regression. No existing caller/test depended on the prior blank-ARN → ResourceNotFoundException behavior; the get_execution_history tests that set durable_execution_arn = "" pass valid input ARNs. Test params are unannotated so the None cases don't trip mypy.
  • Tests adequate. 12 parametrized cases across the four methods, each asserting the exception plus mock_store.load.assert_not_called() (and active_count() == 0 where applicable). As an isolated bug fix with unit coverage, no e2e test is required per AGENTS.md.

Minor non-blocking nit (not filed): the docstrings of get_execution_details/get_execution_history still list only ResourceNotFoundException though they can now surface InvalidParameterValueException via get_execution.

Residual test risk is minimal — the change is small, purely additive input validation, and every wired call site (including the phantom-worker regression) is exercised.

Reviewed commit 9632f071f4c014990057051782c5a23f9205c75c. Workflow run

@yaythomas yaythomas added the needs-triage Issue needs triage label Aug 13, 2026
SravyaPeri and others added 2 commits August 13, 2026 14:45
Co-authored-by: thomas <18520168+yaythomas@users.noreply.github.com>
@yaythomas
yaythomas force-pushed the fix/461-blank-execution-arn-validation branch from b41ffd7 to 033b5fd Compare August 13, 2026 21:45
@yaythomas

Copy link
Copy Markdown
Contributor

thank you so much @SravyaPeri! welcome to dex!

I tweaked the err msg and method name... ,just waiting for CI and then ready to merge.

@yaythomas yaythomas added ready-to-merge and removed needs-triage Issue needs triage labels Aug 13, 2026
@yaythomas

Copy link
Copy Markdown
Contributor

closes #461

@yaythomas
yaythomas merged commit 9e0e3a3 into aws:main Aug 13, 2026
4 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants