Skip to content

Conversation

@Ujjansh05
Copy link
Contributor

@Ujjansh05 Ujjansh05 commented Oct 15, 2025

fix: job_queue β€” set completed_at for terminal statuses and return job copies

πŸ“– Description

This PR fixes two issues in job_queue.py:

  1. Ensures completed_at is set when a job reaches any terminal status ("completed", "failed", or "cancelled").
  2. Returns shallow copies from get_job, get_active_job, and get_pipeline_history to avoid exposing internal job dictionaries to callers β€” preventing accidental external mutation after the lock is released.

Motivation

Previously:

  • Cancelling a job via update_job(..., status="cancelled") did not set completed_at (only cancel_job did).
    β†’ This left cancelled jobs without a completion timestamp.
  • Getter methods returned references to internal dictionaries, which could be mutated by callers, leading to subtle concurrency issues.

πŸ”§ Changes

  • βœ… Set completed_at for all terminal statuses in update_job.
  • βœ… Clear _active_job when a terminal status is set.
  • βœ… Return dict.copy() from:
    • get_job
    • get_active_job
    • get_pipeline_history

Related Issue

_No existing issue; this is a small bugfix and safety improvement.


Testing

  • Ran repository static error check β€” no syntax/type errors found.
  • Full test suite not executed locally due to missing dependency: pytest-asyncio (ModuleNotFoundError).
  • CI will execute the full test matrix.

Details

  • Verified that job_queue.py changes are minimal and logically covered by existing unit tests.

βœ… Checklist

  • Code follows project style guidelines
  • Comments explain β€œwhy,” not β€œwhat”
  • Documentation updated (if needed)
  • No debug code or console statements
  • make format passes
  • make lint passes
  • make typecheck passes
  • make test passes
  • Copilot review run and addressed

Notes

This fix improves reliability and safety in job lifecycle management by ensuring proper completion timestamps and protecting internal state integrity.

@Ujjansh05 Ujjansh05 requested a review from nicofretti as a code owner October 15, 2025 21:22
@nicofretti nicofretti requested a review from Copilot October 16, 2025 06:02
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Sets completed_at for all terminal job statuses (completed, failed, cancelled) and returns shallow copies from job accessors to prevent external mutation of internal state.

  • Set completed_at consistently for all terminal statuses.
  • Clear _active_job on any terminal status.
  • Return shallow copies from get_job, get_active_job, and get_pipeline_history.

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@nicofretti
Copy link
Owner

Hi @Ujjansh05, great catch! Before merging, please ensure you've merged the latest changes from the develop branch. Also, consider adding unit tests to cover this scenario, which will help prevent future regressions. This is excellent work!

@Ujjansh05
Copy link
Contributor Author

Job Queue Tests: Terminal Status & Copy Safety

Summary

Added unit tests to verify correct handling of terminal job statuses and protection against unintended state mutations.

New Tests

test_update_job_sets_completed_at_and_clears_active

  • Ensures update_job():
    • Sets completed_at when status becomes cancelled.
    • Clears the active job when terminal status is reached.

test_getters_return_shallow_copies

  • Confirms get_job() returns a shallow copy.
  • Modifying the returned dict does not affect internal job state.

test_pipeline_history_returns_copies

  • Verifies get_pipeline_history() returns copies of job entries.
  • Mutations to returned history do not alter internal records.

Notes

  • Improves test coverage for correctness and concurrency safety.
  • Ensures job queue’s internal state remains isolated and immutable from external access.

@nicofretti
Copy link
Owner

Nice job! I will take a look asap. I'll merge and close this PR. Feel free to open new issues and contribute!

@Ujjansh05
Copy link
Contributor Author

Thanks for the feedback and for reviewing the PR. I’ll keep an eye out for any follow-up comments or future contributions.

@nicofretti
Copy link
Owner

We can merge! Nice job.

@nicofretti nicofretti merged commit a5b0cea into nicofretti:develop Oct 17, 2025
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.

2 participants