fix: use correct JobResult field names for credential error handling#750
fix: use correct JobResult field names for credential error handling#750KeitaW wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
JobResult has `status` and `message` fields, but four credential-check locations used `success` and `error` — which Pydantic silently ignores, causing credential failures to return SUCCESS instead of FAILED_NO_RETRY. Affected: UploadWorkflowFiles, CleanupWorkflow, UploadApp, DeleteApp. Fixes NVIDIA#748
📝 WalkthroughWalkthroughThe PR fixes incorrect JobResult field usage in four job methods. Previously, credential-missing early returns used non-existent Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/job/jobs.py (1)
382-385: Optional: Consider addingextra = 'forbid'toJobResultinjobs_base.py.As noted in issue
#748, adding Pydantic'sextra = 'forbid'configuration toJobResult(similar toJobExecutionContextat lines 61-63 of this file) would catch incorrect constructor fields at creation time, preventing silent failures in the future.# In src/utils/job/jobs_base.py class JobResult(pydantic.BaseModel): status: JobStatus = JobStatus.SUCCESS message: Optional[str] class Config: extra = 'forbid'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/job/jobs.py` around lines 382 - 385, Add Pydantic strictness to the JobResult model so unknown fields raise errors: update the JobResult class in jobs_base.py to include an inner Config with extra = 'forbid' (similar to JobExecutionContext's Config), ensuring JobResult(...) construction fails fast on unexpected constructor fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/utils/job/jobs.py`:
- Around line 382-385: Add Pydantic strictness to the JobResult model so unknown
fields raise errors: update the JobResult class in jobs_base.py to include an
inner Config with extra = 'forbid' (similar to JobExecutionContext's Config),
ensuring JobResult(...) construction fails fast on unexpected constructor
fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ade3af1-180d-465c-9d88-6e89d89365aa
📒 Files selected for processing (1)
src/utils/job/jobs.py
Summary
Fixes incorrect
JobResultconstructor parameters in four credential-check locations. The code usedsuccess=Falseanderror='...'butJobResultonly hasstatusandmessagefields. Pydantic silently ignores the unknown fields, causing credential failures to returnSUCCESSinstead ofFAILED_NO_RETRY.Affected jobs:
UploadWorkflowFiles.execute()(L382-386)CleanupWorkflow.execute()(L1389-1393)UploadApp.execute()(L1730-1733)DeleteApp.execute()(L1790-1793)Impact: When
workflow_log.credentialorworkflow_app.credentialisNone, the worker logs "completed with status SUCCESS" while no files are uploaded to S3. This makes credential misconfiguration completely invisible.Fix: Replace
success=False, error='...'withstatus=JobStatus.FAILED_NO_RETRY, message='...'— matching the pattern used elsewhere (e.g.,worker.py,backend_jobs.py).Fixes #748
Test plan
workflow_log.credentialunsetFAILED_NO_RETRY: Workflow log credential is not setSummary by CodeRabbit