Skip to content
9 changes: 9 additions & 0 deletions src/deadline/client/ui/controllers/_deadline_controller.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@ class DeadlineUIController(QObject):
queues_updated: Emitted when queue list is updated. Args: [(name, queue_id), ...]
storage_profiles_updated: Emitted when storage profiles are updated.
queue_parameters_updated: Emitted when queue parameters are loaded.
queue_parameters_load_succeeded: Emitted only when a queue parameter fetch
completes successfully (queue_parameters_updated also fires with [] on
fetch errors and for clearing emissions). Args: [parameter, ...] — may
be empty for a queue that genuinely has no queue parameters.
farms_loading: Emitted when farm loading state changes. Args: bool
queues_loading: Emitted when queue loading state changes. Args: bool
storage_profiles_loading: Emitted when storage profile loading state changes.
Expand All @@ -101,6 +105,10 @@ class DeadlineUIController(QObject):
queues_updated = Signal(list)
storage_profiles_updated = Signal(list)
queue_parameters_updated = Signal(list)
# Success-only companion to queue_parameters_updated: not emitted on fetch
# error or for clearing emissions, so consumers that must act only on real,
# successful loads (e.g. CLI --parameter validation) can connect here.
queue_parameters_load_succeeded = Signal(list)

# Emitted after a farm or queue selection has been persisted, so a host dialog
# can reload queue parameters and refresh its Submit button state.
Expand Down Expand Up @@ -495,6 +503,7 @@ def _on_queue_parameters_success(self, parameters: List[JobParameter]) -> None:
"""Handle successful queue parameters fetch."""
self.queue_parameters_loading.emit(False)
self.queue_parameters_updated.emit(parameters)
self.queue_parameters_load_succeeded.emit(parameters)

def _on_queue_parameters_error(self, error: BaseException) -> None:
"""Handle queue parameters fetch error."""
Expand Down
47 changes: 39 additions & 8 deletions src/deadline/client/ui/job_bundle_submitter.py
Original file line number Diff line number Diff line change
Expand Up @@ -411,20 +411,51 @@ def on_create_job_bundle_callback(
)

if job_parameters:
# We want to validate the job parameters after the queue parameters are loaded.
# Connect a parameter validation function to the queue parameter loading completion
def validate_parameters_after_queue_load(refresh_id: int, queue_parameters: list):
# The controller is a global singleton that outlives this dialog, and its
# queue_parameters_updated signal also fires with [] to clear stale state
# (farm/queue switch, fetch error, nothing selected) — indistinguishable
# from a queue that genuinely has zero queue parameters. Connect to the
# success-only queue_parameters_load_succeeded signal instead, so we only
# validate against a real, successful load (which may legitimately be
# empty) and keep waiting through clears and transient fetch errors.
# Validate single-shot, then disconnect so the closure over
# submitter_dialog can't fire against a closed dialog later.
controller = submitter_dialog.shared_job_settings._controller
# Mutable cell so both closures below share the connection state. Guards
# the double-disconnect (validation ran, then the dialog is destroyed),
# which PySide6 reports with a RuntimeWarning rather than an exception.
connected = [False]

def disconnect_validation_callback():
if not connected[0]:
return
connected[0] = False
try:
controller.queue_parameters_load_succeeded.disconnect(
validate_parameters_after_queue_load
)
except (TypeError, RuntimeError):
# Some bindings raise instead of warn when already disconnected.
pass

def validate_parameters_after_queue_load(queue_parameters: list):
"""Validate CLI parameters against loaded queue parameters and set parameter values"""
disconnect_validation_callback()
if not _validate_and_warn_about_parameters(
job_parameters, initial_settings.parameters, queue_parameters, submitter_dialog
):
# User chose to cancel, close the dialog
# User cancelled at the validation warning.
submitter_dialog.close()

# Connect to the queue parameters update signal
submitter_dialog.shared_job_settings._queue_parameters_update.connect(
validate_parameters_after_queue_load
)
# Validate CLI params once the controller successfully loads queue params.
controller.queue_parameters_load_succeeded.connect(validate_parameters_after_queue_load)
connected[0] = True
# If the dialog goes away before any load succeeds, tear the connection down.
# The dialog does not set WA_DeleteOnClose, so an ordinary close never emits
# destroyed — hook finished (emitted on accept/reject/close of a visible
# dialog) for the close path, and destroyed for deletion without done().
submitter_dialog.finished.connect(lambda _result: disconnect_validation_callback())
submitter_dialog.destroyed.connect(disconnect_validation_callback)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The cleanup relies on submitter_dialog.destroyed to tear down the connection to the singleton controller, but destroyed only fires when the QObject is actually deleted, not when the dialog is merely closed/hidden. SubmitJobToDeadlineDialog does not set WA_DeleteOnClose (nor call deleteLater), so a user who closes the dialog before the first successful queue-parameter load leaves this connection live on the long-lived singleton.

If any other consumer then triggers a successful queue-parameters fetch, validate_parameters_after_queue_load fires against the closed-but-alive dialog: it pops a QMessageBox parented to a dead window and may call submitter_dialog.close() on it. The comment claims the disconnect protects against firing against a closed dialog, but that only holds for the single-shot self-disconnect after a load, not for a plain close before any load.

Note that test_dialog_destroyed_disconnects_validator emits destroyed explicitly, so it does not exercise the ordinary close path where destroyed never fires. Consider tying the teardown to the dialog close event (or setting WA_DeleteOnClose) rather than destroyed.

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.

Confirmed: SubmitJobToDeadlineDialog does not set WA_DeleteOnClose, and the gui-submit CLI path holds a reference through app.exec(), so an ordinary close never emits destroyed and the connection would have stayed live on the singleton.

The teardown is now also connected to QDialog.finished, which fires on accept/reject/close of a shown dialog, so close-before-any-load disconnects the callback immediately. destroyed remains connected to cover deletion without done(), and a shared connected-state guard makes all three teardown paths (single-shot self-disconnect, finished, destroyed) idempotent.

The test stand-in dialog is now a real QDialog (no WA_DeleteOnClose, matching production), and test_dialog_closed_before_load_disconnects_validator shows and closes it through the normal close path — no destroyed emission — then asserts a subsequent queue_parameters_load_succeeded emission does not invoke the validator or touch the closed dialog. This test fails without the finished hook.


submitter_dialog.show()
return submitter_dialog
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,72 @@ def test_select_storage_profile_persists_without_cascade(
mock_api.list_storage_profiles_for_queue.assert_not_called()
mock_api.get_queue_parameter_definitions.assert_not_called()

@patch("deadline.client.ui.controllers._deadline_controller.api")
def test_queue_params_success_emits_load_succeeded(
self, mock_api, qtbot, fresh_deadline_config
):
"""A successful fetch emits queue_parameters_load_succeeded with the parameters,
even when the queue genuinely has zero queue parameters."""
controller = DeadlineUIController.getInstance()
mock_api.get_queue_parameter_definitions.return_value = []

succeeded = []
updated = []
controller.queue_parameters_load_succeeded.connect(
lambda x: succeeded.append(x), _QueuedConnection
)
controller.queue_parameters_updated.connect(lambda x: updated.append(x), _QueuedConnection)

controller.refresh_queue_parameters(farm_id="farm-123", queue_id="queue-456")

qtbot.waitUntil(lambda: len(succeeded) > 0, timeout=2000)
assert succeeded[0] == []
assert updated[0] == []

@patch("deadline.client.ui.controllers._deadline_controller.api")
def test_queue_params_error_does_not_emit_load_succeeded(
self, mock_api, qtbot, fresh_deadline_config
):
"""A failed fetch emits queue_parameters_updated([]) but NOT load_succeeded, so
success-only consumers can distinguish an error from an empty-but-real load."""
controller = DeadlineUIController.getInstance()
mock_api.get_queue_parameter_definitions.side_effect = RuntimeError("boom")

succeeded = []
updated = []
controller.queue_parameters_load_succeeded.connect(
lambda x: succeeded.append(x), _QueuedConnection
)
controller.queue_parameters_updated.connect(lambda x: updated.append(x), _QueuedConnection)

controller.refresh_queue_parameters(farm_id="farm-123", queue_id="queue-456")

qtbot.waitUntil(lambda: len(updated) > 0, timeout=2000)
assert updated[0] == []
assert succeeded == []

@patch("deadline.client.ui.controllers._deadline_controller.api")
def test_clearing_emission_does_not_emit_load_succeeded(
self, mock_api, qtbot, fresh_deadline_config
):
"""refresh with no farm/queue selected clears via queue_parameters_updated([]) without
emitting load_succeeded."""
controller = DeadlineUIController.getInstance()

succeeded = []
updated = []
controller.queue_parameters_load_succeeded.connect(
lambda x: succeeded.append(x), _QueuedConnection
)
controller.queue_parameters_updated.connect(lambda x: updated.append(x), _QueuedConnection)

controller.refresh_queue_parameters(farm_id="", queue_id="")

qtbot.waitUntil(lambda: len(updated) > 0, timeout=2000)
assert updated[0] == []
assert succeeded == []
mock_api.get_queue_parameter_definitions.assert_not_called()

def test_shutdown_cancels_operations(self, qtbot):
"""Test that shutdown cancels pending operations."""
controller = DeadlineUIController.getInstance()
Expand Down
Loading
Loading