Skip to content

Fix job attachment 401s for submitted jobs - #773

Draft
v-elegacheva wants to merge 15 commits into
microsoft:mainfrom
v-elegacheva:ekaterina/sdk-job-update-e2e
Draft

v-elegacheva wants to merge 15 commits into
microsoft:mainfrom
v-elegacheva:ekaterina/sdk-job-update-e2e

Conversation

@v-elegacheva

@v-elegacheva v-elegacheva commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes job attachment operations failing after submission because the service stores an unsigned container URI.

upload_attachment, download_attachment, and list_attachments now:

  • Preserve explicitly supplied method-level container URIs.
  • Request a workspace-issued SAS for the job's actual linked container on first use.
  • Cache and revalidate that SAS for HTTPS, container identity, the backend's fixed attachment capabilities (r, w, and l), start time, and expiry.
  • Invalidate cache use when job container details change.
  • Reject a refreshed URI when its storage hostname differs from the hostname stored in the job details.

Custom container names remain supported. The legacy Workspace(storage=...) connection-string path is intentionally outside this PR's scope; its deprecation will be tracked separately.

Testing

  • Focused attachment validation: 25 passed.
  • Full local suite: 127 passed, 18 skipped.
  • Azure Quantum Python CI for the previous revision: build 183619 passed. CI will be rerun for the final revision.
  • The relevant DPv2 attachment E2E in build 183473 passed upload, list, and download (34 passed, 2 skipped in the DPv2 SDK job). The broader live build had unrelated CLI and third-party failures. The attachment E2E will be rerun against the final revision.

Related

The generated job-update endpoint correction is intentionally left to #772, which regenerated the complete client with the corrected /jobs/{jobId} route.

- upload_attachment, download_attachment, and list_attachments in base_job.py were reusing the unsigned container_uri returned by job creation instead of always fetching a fresh SAS-signed URI via workspace.get_container_uri(), causing 401 NoAuthenticationInformation errors.
- build_services_jobs_update_request in the generated _operations.py used the wrong URL path segment 'jobUpdateOptions' (the request model type name) instead of 'jobs', causing workspace.update_job() to fail with 404 Not Found.
@v-elegacheva
v-elegacheva requested a review from a team as a code owner September 18, 2026 00:00
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@v-elegacheva

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@v-elegacheva
v-elegacheva marked this pull request as draft September 18, 2026 17:45
@v-elegacheva

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@v-elegacheva

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Attachment methods now target the wrong container for jobs created with a custom container name.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes attachment authentication failures and the incorrect job-update endpoint.

Changes:

  • Refreshes SAS-signed attachment container URIs.
  • Corrects job PATCH requests to use /jobs/{jobId}.
  • Adds regression tests for both fixes.
File summaries
File Description
base_job.py Refreshes attachment container SAS URIs.
_operations.py Corrects the job update route.
test_job_attachments.py Tests attachment URI handling.
test_workspace.py Tests the PATCH resource path.
Review details

Suppressed comments (2)

azure-quantum/azure/quantum/job/base_job.py:381

  • This selects the default job-{id} container rather than the container encoded in self._details.container_uri. Jobs created with a custom container_name will therefore download from the wrong location. Request the fresh SAS using self.container_name and update the test expectation accordingly.
            container_uri = self.workspace.get_container_uri(job_id=self.id)

azure-quantum/azure/quantum/job/base_job.py:400

  • Listing now ignores the job's actual linked container name and always targets job-{id}. This breaks list_attachments() for the supported custom-container flow in from_input_data; obtain the fresh SAS for self.container_name and cover that case in the attachment tests.
        container_uri = self.workspace.get_container_uri(job_id=self.id)
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread azure-quantum/azure/quantum/job/base_job.py Outdated
@v-elegacheva

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@v-elegacheva v-elegacheva changed the title Fix upload/download/list attachment 401s and job update 404 Fix job attachment 401s for submitted jobs Sep 18, 2026
@v-elegacheva

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Caller-supplied containers from different storage accounts can be redirected to same-named workspace containers.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

azure-quantum/azure/quantum/job/base_job.py:387

  • For jobs created with from_storage_uri(..., container_uri=...) against another storage account, this re-signs only the extracted container name in the workspace's configured account. The download then reads a different container rather than the job's explicitly linked one. Preserve a usable explicit job-level URI, or restrict workspace SAS refresh to URIs known to be in workspace storage.
            container_uri = self.workspace.get_container_uri(
                job_id=self.id,
                container_name=self.container_name,
            )

azure-quantum/azure/quantum/job/base_job.py:409

  • This loses the storage-account identity from a caller-supplied job container_uri: get_container_uri selects the workspace-linked/configured account using only the name. Consequently, list_attachments() can list an unrelated same-named container (or create and list an empty one) instead of the explicit container retained by from_storage_uri. Preserve a usable explicit URI or verify the account before refreshing its SAS through the workspace.
        container_uri = self.workspace.get_container_uri(
            job_id=self.id,
            container_name=self.container_name,
        )
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread azure-quantum/azure/quantum/job/base_job.py Outdated
@v-elegacheva

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Permission-specific validation and reuse of valid job-level SAS URIs are not implemented correctly.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (3)

Comment thread azure-quantum/azure/quantum/job/base_job.py

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

A valid job-level SAS is unnecessarily refreshed instead of reused.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Reuse usable stored SAS URI before refreshing container URI

azure-quantum/​azure/​quantum/​job/​base_job.py:420

A usable SAS already stored in JobDetails.container_uri is never considered here: on a new Job, the cache is empty, so even an unexpired signed URI always calls workspace.get_container_uri. This contradicts the stated reuse behavior and can fail for caller-supplied/external containers even though the existing SAS is sufficient. Cache and return the stored URI when _is_attachment_container_uri_usable(container_uri) succeeds before refreshing it; update the current signed-job-URI test, which presently expects the unnecessary refresh.

@v-elegacheva

Copy link
Copy Markdown
Contributor Author

Copilot review overview

🔵 Needs a closer look

A valid job-level SAS is unnecessarily refreshed instead of reused.

Review effort: Balanced Findings: None

Resolved since last review (1)

Previously missed (1)
In code that hasn't changed since last review

Medium severity Reuse usable stored SAS URI before refreshing container URI

This refresh is intentional. Current Dpv1 and Dpv2 Production flows store unsigned job container URIs and require workspace-linked storage, so JobDetails.container_uri is used only to identify the expected linked container and storage hostname.
The first attachment operation requests a workspace-issued SAS, and subsequent operations reuse the validated per-job cache. Caller-supplied external job containers are outside the supported scope, while explicit method-level uri overrides remain unchanged

@v-elegacheva

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

token_permissions = set(query_params.get("sp", [""])[0])
if (
not query_params.get("sig")
or not _ATTACHMENT_CONTAINER_SAS_PERMISSIONS.issubset(token_permissions)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is the inverse of what I suggested. My argument was that permission checking is unproductive — the SDK can't request permissions (BlobDetails has only containerName/blobName), so a refresh can never remedy a permission gap. The conclusion was to drop the check and let storage answer authoritatively. Instead the check became a stricter, global precondition that fails harder. test_upload_attachment_rejects_refreshed_uri_without_full_attachment_permissions now pins rl-rejection as intended behavior.

Suggestion: validate only what a refresh can actually fix — sig present and not near expiry (the st handling is a genuinely good addition, keep it). Let a permission shortfall surface as the storage service's own 403, which names the real cause.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

addressed. I removed the permission checking entirely + sig, HTTPS, hostname and path validation. The cache now checks only SAS expiry. A permission shortfall now surfaces as storage's own error instead of sdk precondition

def _get_attachment_container_uri(self) -> str:
"""Return a validated workspace-issued SAS URI for the job's attachment container.

The first call refreshes the unsigned URI stored in job details. Later calls reuse the

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we shouldn't set container sas uri to job details right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed so. I checked again- the refreshed sas is only stored in a private per-job cache and is never written back to job.details.container_uri. Updated in recent commit

container_uri: Optional[str],
) -> tuple[Optional[str], str]:
if container_uri is None:
return (None, f"/job-{self.id}")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

_get_attachment_container_identity hardcodes f"/job-{self.id}", which is a third copy of the job-{id} convention (also in container_name and workspace.get_container_uri's container_name_format)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I removed _get_attachment_container_identity() completely. The cache is now keyed on the existing container_name property, so there is no separate job-{id} formatting duplicated for the cache

Comment on lines +442 to +445
if not self._is_attachment_container_uri_usable(refreshed_container_uri):
raise ValueError(
"Refreshed attachment container URI does not contain a usable SAS token."
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we can skip this check as we assume that backend always return usable sas uri

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay! The check has been removed

Comment on lines +435 to +440
if stored_hostname != refreshed_hostname:
raise ValueError(
"Refreshed attachment container hostname "
f"'{refreshed_hostname}' does not match job container hostname "
f"'{stored_hostname}'."
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do we need to compare hostnames here? we are operating with workspace with linked storage here and it won't change for the workspace lifecycle right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I removed that. I confirmed with backend that dpv2 production and all sas-issuing endpoints validate the account name against the workspace's linked storage account server-side, so this check was indeed redundant for the linked-storage flows

Comment on lines +463 to +510
def _is_attachment_container_uri_usable(
self,
container_uri: str,
) -> bool:
"""Check whether a workspace-issued container SAS can serve all attachment operations."""

parsed_uri = urlparse(container_uri)
if (
parsed_uri.scheme.lower() != "https"
or parsed_uri.hostname is None
or not parsed_uri.path.strip("/")
):
return False

query_params = parse_qs(parsed_uri.query)
token_expire_query_param = query_params.get("se")
token_start_query_param = query_params.get("st")
token_permissions = set(query_params.get("sp", [""])[0])
if (
not query_params.get("sig")
or not _ATTACHMENT_CONTAINER_SAS_PERMISSIONS.issubset(token_permissions)
or not token_expire_query_param
):
return False

try:
token_expire_time = datetime.fromisoformat(
token_expire_query_param[0].replace("Z", "+00:00")
)
if token_expire_time.tzinfo is None:
token_expire_time = token_expire_time.replace(tzinfo=timezone.utc)

token_start_time = None
if token_start_query_param:
token_start_time = datetime.fromisoformat(
token_start_query_param[0].replace("Z", "+00:00")
)
if token_start_time.tzinfo is None:
token_start_time = token_start_time.replace(tzinfo=timezone.utc)

current_utc_time = datetime.now(tz=timezone.utc)
has_started = token_start_time is None or token_start_time <= current_utc_time
return has_started and current_utc_time + timedelta(minutes=5) < token_expire_time
except ValueError:
logger.debug(
"Unable to parse attachment SAS start or expiry time; requesting a fresh URI."
)
return False

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

narrow this method to the one condition a refresh can genuinely fix — expiry — and let storage adjudicate the rest.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done! The cache reuse check (renamed to _is_attachment_container_uri_unexpired) now validated only se expiry with the 5 min buffer.
sig, st and permissions are not checked anymore

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Cached and refreshed URIs lack required identity, security, start-time, signature, and permission validation.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)

Comment thread azure-quantum/azure/quantum/job/base_job.py
@v-elegacheva

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Cached and refreshed URIs lack the promised identity, permission, start-time, signature, and HTTPS validation.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (1)

Comment on lines +433 to +435
self._attachment_container_uri_cache = refreshed_container_uri
self._attachment_container_uri_cache_container_name = container_name
return refreshed_container_uri

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Wrong-account/ HTTP: Not reachable through this code path. Workspace.get_container_uri()'s signiture is (job-id,contianer_name, container_name_format). It never reads a hostname and resolves to exactlyone account (self.storage) on the Workspace instance), confimed by test_cache_reuse_after_hostname_mutation_never_queries_by_hostname. Backend-issued sas URIs are also always built from BlobServiceClient / BlobContainerClient tied to the account's https:// blob endpoint so an http:// response isnt something the backend can structurally return for this flow.

Unsigned / insufficient-permission accepted: Not an oversight. Per feedback - agreed to narrow validation to the one condition a refresh can fix (expiry) and defer signature/permission enforcement to Blob Storage's own authorative response. Backend-issued tokens are always signed with full permissions in practice, so this only affects synthetic/malformed inputs, which several tests exercise to document the pass-through behavior

@v-elegacheva

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants