Skip to content

fix: avoid crashes in job bundle submission edge cases - #1290

Merged
crowecawcaw merged 6 commits into
aws-deadline:mainlinefrom
crowecawcaw:review-fix/submit-bundle-crashes
Jul 25, 2026
Merged

fix: avoid crashes in job bundle submission edge cases#1290
crowecawcaw merged 6 commits into
aws-deadline:mainlinefrom
crowecawcaw:review-fix/submit-bundle-crashes

Conversation

@crowecawcaw

@crowecawcaw crowecawcaw commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Fixes:

What was the problem/requirement? (What/Why)

Three crash/robustness bugs in job-bundle submission (api/_submit_job_bundle.py):

  • asset_manager was only assigned inside the attachments block but passed unconditionally to _save_debug_snapshotUnboundLocalError when submitting a bundle with no attachments and a debug snapshot.
  • job["lifecycleStatusMessage"] was read without a fallback → KeyError made a successful submit look like a crash.
  • config wasn't forwarded to _upload_attachments/_snapshot_attachments, so telemetry used the wrong config.

What was the solution? (How)

  • Initialize asset_manager = None before the conditional and accept Optional[S3AssetManager] in _save_debug_snapshot (it's only dereferenced inside the attachments branch).
  • Use job.get("lifecycleStatusMessage", "") in both waiter branches.
  • Forward config=config to both attachment helpers.

What is the impact of this change?

Debug-snapshot submissions without attachments no longer crash, successful submits report success, and attachment telemetry uses the correct config.

How was this change tested?

Added red-green unit tests for each: no-attachments debug snapshot, missing lifecycleStatusMessage, and config forwarded to the attachment helpers.

  • Have you run the unit tests? Yestest_job_bundle_submission.py and adjacent bundle-submit tests (58 passed).
  • Have you run the integration tests? No.

Was this change documented?

  • No public-contract change.
  • README.md not affected.

Does this PR introduce new dependencies?

  • This PR adds one or more new dependency Python packages.
  • This PR does not add any new dependencies.

Is this a breaking change?

No.

Does this change impact security?

No. (Known-path containment (C-known-path) and the stack-trace sanitizer are handled in separate PRs; the session-context race and debug-snapshot command-injection findings are out of scope here.)

Testing

All verification is automated unit tests (no live AWS calls; deadline/boto3 clients mocked), each proven red-green against the pre-fix parent commit:

Fix (a) — asset_manager UnboundLocalError:

  • test_create_job_from_job_bundle_debug_snapshot_no_attachments: debug snapshot of a bundle with no attachments completes cleanly; snapshot files are written, no job is submitted, and the generated submit_job.sh contains aws deadline create-job but correctly omits aws s3 cp lines. Pre-fix failure: UnboundLocalError: cannot access local variable 'asset_manager'.
  • test_create_job_from_job_bundle_debug_snapshot_with_attachments (new): the other side of the Optional[S3AssetManager] change — a snapshot with attachments uses _snapshot_attachments (not _upload_attachments) with a bound asset manager, writes attachments into create_job_args.json, and emits the aws s3 cp upload commands in the generated script.

Fix (b) — missing lifecycleStatusMessage:

  • test_wait_for_create_job_to_complete_missing_status_message, parametrized over both waiter branches: a get_job response omitting lifecycleStatusMessage returns (True, "") for READY and (False, "") for CREATE_FAILED. Pre-fix failure: KeyError: 'lifecycleStatusMessage' in each branch. (Note: the shared MockDeadlineBackend used by UI/e2e tests always includes lifecycleStatusMessage, so this unit test is the permanent coverage for the omission case.)

Fix (c) — config forwarding:

  • test_create_job_from_job_bundle_forwards_config_to_upload_attachments: normal submit forwards the caller's config object to _upload_attachments (verified by identity, is). Pre-fix failure: config absent from the call kwargs.
  • test_create_job_from_job_bundle_debug_snapshot_with_attachments (new) verifies the same for _snapshot_attachments on the snapshot path; it fails pre-fix with config absent.

Suite runs: test_job_bundle_submission.py (41 tests) plus the full test/unit/deadline_client/api/ + test_cli_bundle_submit*.py suites (578 passed). hatch run fmt and hatch run lint (ruff check, ruff format, mypy) pass; full CI matrix is green.

Not covered (manual-only, not automatable): executing a generated submit_job.sh/submit_job.bat against a live farm, and observing a real CreateJob whose GetJob response omits lifecycleStatusMessage. These are optional hardening checks; the mocked flows exercise the exact code paths changed.

When create_job_from_job_bundle is called with debug_snapshot_dir for a
bundle that has no job attachments, the attachments block never runs, so
the asset_manager local is never assigned. It was then passed
unconditionally to _save_debug_snapshot, raising UnboundLocalError and
making snapshotting crash.

Initialize asset_manager to None before the conditional and accept
Optional[S3AssetManager] in _save_debug_snapshot, which only dereferences
it when the create_job args carry attachments.

Signed-off-by: Stephen Crowe <[email protected]>
wait_for_create_job_to_complete read job["lifecycleStatusMessage"] with
subscript access on both the success and failure branches. A get_job
response that omits that field raised KeyError, turning an otherwise
successful submission into a crash. Read it with .get(..., "") so a
missing message degrades to an empty string instead of raising.

Signed-off-by: Stephen Crowe <[email protected]>
_upload_attachments and _snapshot_attachments both accept a config
parameter, and _upload_attachments uses it to select the telemetry
client for the upload summary. create_job_from_job_bundle never passed
its config through, so upload telemetry used the default on-disk config
instead of the caller-supplied one. Forward config to both call sites.

Signed-off-by: Stephen Crowe <[email protected]>
@github-actions github-actions Bot added the waiting-on-maintainers Waiting on the maintainers to review. label Jul 21, 2026
Adds a unit test for the debug-snapshot-WITH-attachments combination:
verifies _snapshot_attachments is used (not _upload_attachments), receives
the caller's config, the snapshot's create_job_args.json contains the
attachments, and the generated submit_job.sh includes the 'aws s3 cp'
upload commands. This locks in the non-None side of the Optional
asset_manager change and the config forwarding on the snapshot path.

Also tightens the no-attachments snapshot test to assert the generated
script omits 'aws s3 cp' while still containing the create-job command.

Red-green: the new test fails on the pre-fix parent commit because
_snapshot_attachments was called without config.

Signed-off-by: Stephen Crowe <[email protected]>
@crowecawcaw
crowecawcaw marked this pull request as ready for review July 24, 2026 16:33
@crowecawcaw
crowecawcaw requested a review from a team as a code owner July 24, 2026 16:33
@crowecawcaw
crowecawcaw enabled auto-merge (squash) July 25, 2026 23:41
asset_manifests,
print_function_callback,
upload_progress_callback,
config=config,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike _upload_attachments (which forwards config to get_deadline_cloud_library_telemetry_client(config=config) at line 220), _snapshot_attachments never uses its config parameter — its body does not reference it, and its @record_success_fail_telemetry_event decorator calls get_deadline_cloud_library_telemetry_client() with no config. So the config=config added here is effectively dead, and the new test docstring claiming the config is "forwarded ... for telemetry" is inaccurate for the snapshot path. Either wire config into the telemetry client inside _snapshot_attachments (to match upload behavior) or drop the parameter and the corresponding test assertion to avoid implying behavior that does not exist.

@crowecawcaw
crowecawcaw merged commit d054001 into aws-deadline:mainline Jul 25, 2026
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-on-maintainers Waiting on the maintainers to review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants