Handle DRMAA failures as terminal job statuses - #485
Merged
Conversation
BaseDrmaaManager._get_status_external() mapped JobState.FAILED to status.COMPLETE, with a comment acknowledging it should map to FAILED instead. This means any job that fails at the Slurm/DRMAA submission or execution level (e.g. rejected for a missing/invalid account) is reported back to Galaxy as a successful completion, with no output and no indication anything went wrong. status.FAILED already exists and is treated identically to status.COMPLETE by status.is_job_done(), so this only affects the terminal status Galaxy receives, not job-done detection.
StatefulManagerProxy only recognized COMPLETE and CANCELLED as terminal, so a manager returning FAILED left the job active forever: never deactivated, polled by ManagerMonitor for the life of the process, its active_jobs entry re-registered on restart, and _deactivate_job never called on the proxied manager. Deactivating is only half of it. The terminal notification lived inside do_postprocess, which ran only for COMPLETE, so even a deactivated FAILED job told the client nothing - and message-driven clients never poll, so the job hangs in running rather than being wrongly reported complete. FAILED now postprocesses like COMPLETE, which preserves the staging the old FAILED -> COMPLETE mapping gave remote_transfer deployments and reuses the existing stage-then-notify ordering. CANCELLED still notifies nobody, because the client initiated it and already knows. status.is_job_done() looks like the obvious thing to reuse here and it is not: it also counts LOST, which ExternalBaseManager returns for a job whose external id has not been recovered yet. PulsarApp binds the message queue - and so starts the monitor - before recover_active_jobs runs, so making LOST terminal would race a restart into abandoning healthy jobs. Named the terminal set instead and said why. Also stop the preprocessing-failure path from re-deactivating on every poll; it signals a state change once now, and does not stage or notify a second time for a job that never launched. The gap predates the DRMAA change - queued_cli has been able to return FAILED since 2022. integration_test_state.test_restart_finishes_job kills the job through the DRMAA session rather than the manager, so it is a DRM-side failure; its expectation encoded the bug and becomes "failed".
ExternalDrmaaQueueManager chowned the working directory back to the Pulsar user only on COMPLETE. A job that fails DRM-side is just as done with the directory, and without the chown Pulsar can neither read outputs nor clean up.
Covers the whole table rather than the one entry that changed - a state the stateful manager does not recognize as terminal leaves the job hanging. Also fixes tearDown calling setUp, which leaked a staging directory per test.
jmchilton
marked this pull request as ready for review
August 18, 2026 20:24
get_status starts postprocessing on its own thread and reports POSTPROCESSING only while that thread has not finished. These jobs stage nothing, so the thread can finish first and get_status correctly returns the terminal status - which made every assertion on POSTPROCESSING a race. It lost on CI: the same commit passed in one run and failed three tests in another. Hold postprocessing on an event the test releases after observing the state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
JobState.FAILEDto Pulsar'sfailedstatusStatefulManagerProxyThis builds on and supersedes #482. The original mapping fix is retained as its own commit with Govind's authorship, and the changelog credits @gkr0110.
Root cause
PR #482 corrects the DRMAA-to-Pulsar mapping, but
StatefulManagerProxypreviously recognized onlycompleteandcancelledas terminal. A DRMAA failure would therefore remain active, continue being polled, and never notify message-driven clients. The external DRMAA manager also reclaimed directory ownership only for successful completion.The stateful terminal path now handles
failedexplicitly while leavinglostrecoverable during startup. Both successful and failed jobs are postprocessed before their terminal callback is sent.User impact
DRM-side failures such as scheduler submission rejection, walltime expiration, or resource-limit termination are reported as failed instead of complete. Message-driven Galaxy clients receive the terminal update, partial outputs can still be staged, and Pulsar releases the associated manager and working-directory state.
Validation
tox -e test-unit -- test/stateful_test.py test/manager_drmaa_test.py— 6 passed, 3 skipped locally because no DRMAA library is configuredpulsarandtestgit diff --checkpassed