Skip to content

job-manager: avoid posting redundant and failing dependency events - #7885

Merged
mergify[bot] merged 8 commits into
flux-framework:masterfrom
grondo:job-manager-dependency-fixes
Nov 2, 2026
Merged

mergify[bot] merged 8 commits into
flux-framework:masterfrom
grondo:job-manager-dependency-fixes

Conversation

@grondo

@grondo grondo commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

Problem: flux_jobtap_dependency_add() and flux_jobtap_dependency_remove() can post events that have no effect or fail to apply. This happens when a plugin re-adds or removes dependencies on restart or plugin reload, or adds and removes a dependency in the same callback. In a callback, a remove of a dependency that was never added returns success, so the plugin does not get the error.

This PR makes repeated add and remove calls post no event, and makes a remove of a dependency that was never added fail with ENOENT. It also corrects flux-jobtap-plugins(7), which says a repeated add returns EEXIST.

This came out of the review of #7877. The related job manager fix for events that fail to apply is in #7881. This addresses the comment at the end of #7868.

@garlick garlick left a comment

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.

LGTM!

@grondo
grondo force-pushed the job-manager-dependency-fixes branch from b360a5d to b55d5b0 Compare November 1, 2026 23:03
Problem: The job manager cannot check whether a dependency event for a
description is queued.

Add a function that checks the job event queue.

Assisted-by: Claude:opus-5.5
Problem: Adding a dependency that was removed posts a dependency-add
event that has no effect. Plugins do this on restart and plugin reload.
Adding a dependency twice in one callback also posts two events.

Post no event if the description is used or queued for add.

Assisted-by: Claude:opus-5.5
Problem: Removing a dependency that was already removed posts a
dependency-remove event that fails to apply.

Post no event if the dependency is removed or queued for removal.

Assisted-by: Claude:opus-5.5
Problem: In a callback, removing a description that was never added
returns success. The event fails to apply later, so the plugin does not
get the error.

Fail with ENOENT before posting the event, unless an add for the
description is queued.

Assisted-by: Claude:opus-5.5
Problem: The flux-jobtap-plugins(7) man page says that a repeated
dependency add returns EEXIST. It does not describe a repeated remove
or a remove of a dependency that was never added.

Correct the description of add and describe both remove cases.

Assisted-by: Claude:opus-5.5
Problem: The dependency-test plugin cannot remove a dependency more
than once in a callback.

Treat the remove option as a count.

Assisted-by: Claude:opus-5.5
Problem: The dependency-test plugin cannot remove a dependency that was
never added.

Add the system.dependency-test-absent attribute. In job.state.depend,
remove the named dependency and raise an exception if the remove does
not fail with ENOENT.

Assisted-by: Claude:opus-5.5
Problem: No test covers a repeated dependency add or remove, or the
removal of a dependency that was never added.

Add tests to t2270-job-dependencies.t.

Assisted-by: Claude:opus-5.5
@grondo
grondo force-pushed the job-manager-dependency-fixes branch from b55d5b0 to f22abb1 Compare November 2, 2026 00:43
@mergify mergify Bot added the queued label Nov 2, 2026
@mergify
mergify Bot merged commit ff4072a into flux-framework:master Nov 2, 2026
36 of 38 checks passed
@mergify

mergify Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

✅ Merged into master as ff4072a at 01:17 UTC

Less than a minute in the queue, with no time running CI.

So far

  • ✅ Entered the queue Mon 2 Nov, 01:17 · rule default · triggered by rule rebase and merge when passing all checks
  • ✅ Checks skipped · PR is already up-to-date
  • ✅ Merged 01:17 · ff4072a · merge
25 merge conditions met
  • github-review-approved · required by GitHub branch protection
  • any of · required by GitHub branch protection:
    • check-success = validate commits
    • check-neutral = validate commits
    • check-skipped = validate commits
  • any of · required by GitHub branch protection:
    • check-success = address-sanitizer check
    • check-neutral = address-sanitizer check
    • check-skipped = address-sanitizer check
  • any of · required by GitHub branch protection:
    • check-success = coverage
    • check-neutral = coverage
    • check-skipped = coverage
  • any of · required by GitHub branch protection:
    • check-success = focal - py3.8
    • check-neutral = focal - py3.8
    • check-skipped = focal - py3.8
  • any of · required by GitHub branch protection:
    • check-success = docs/readthedocs.org:flux-core
    • check-neutral = docs/readthedocs.org:flux-core
    • check-skipped = docs/readthedocs.org:flux-core
  • any of · required by GitHub branch protection:
    • check-success = inception
    • check-neutral = inception
    • check-skipped = inception
  • any of · required by GitHub branch protection:
    • check-success = flux-sched check
    • check-neutral = flux-sched check
    • check-skipped = flux-sched check
  • any of · required by GitHub branch protection:
    • check-success = el8 - system,coverage
    • check-neutral = el8 - system,coverage
    • check-skipped = el8 - system,coverage
  • any of · required by GitHub branch protection:
    • check-success = spelling
    • check-neutral = spelling
    • check-skipped = spelling
  • any of · required by GitHub branch protection:
    • check-success = el8 - ascii
    • check-neutral = el8 - ascii
    • check-skipped = el8 - ascii
  • any of · required by GitHub branch protection:
    • check-success = bookworm - 32 bit
    • check-neutral = bookworm - 32 bit
    • check-skipped = bookworm - 32 bit
  • any of · required by GitHub branch protection:
    • check-success = flux-accounting check
    • check-neutral = flux-accounting check
    • check-skipped = flux-accounting check
  • any of · required by GitHub branch protection:
    • check-success = python linting
    • check-neutral = python linting
    • check-skipped = python linting
  • any of · required by GitHub branch protection:
    • check-success = el9 - test-install
    • check-neutral = el9 - test-install
    • check-skipped = el9 - test-install
  • any of · required by GitHub branch protection:
    • check-success = fedora40 - clang-18
    • check-neutral = fedora40 - clang-18
    • check-skipped = fedora40 - clang-18
  • any of · required by GitHub branch protection:
    • check-success = noble - test-install
    • check-neutral = noble - test-install
    • check-skipped = noble - test-install
  • any of · required by GitHub branch protection:
    • check-success = alpine - test-install
    • check-neutral = alpine - test-install
    • check-skipped = alpine - test-install
  • any of · required by GitHub branch protection:
    • check-success = fedora40 - test-install
    • check-neutral = fedora40 - test-install
    • check-skipped = fedora40 - test-install
  • any of · required by GitHub branch protection:
    • check-success = bookworm - test-install
    • check-neutral = bookworm - test-install
    • check-skipped = bookworm - test-install
  • any of · required by GitHub branch protection:
    • check-success = jammy - test-install
    • check-neutral = jammy - test-install
    • check-skipped = jammy - test-install
  • any of · required by GitHub branch protection:
    • check-success = flux-pam check
    • check-neutral = flux-pam check
    • check-skipped = flux-pam check
  • any of · required by GitHub branch protection:
    • check-success = flux-pmix check
    • check-neutral = flux-pmix check
    • check-skipped = flux-pmix check
  • any of · required by GitHub branch protection:
    • check-success = bookworm - gcc-12,distcheck
    • check-neutral = bookworm - gcc-12,distcheck
    • check-skipped = bookworm - gcc-12,distcheck
  • any of · required by GitHub branch protection:
    • check-success = el10 - test-install
    • check-neutral = el10 - test-install
    • check-skipped = el10 - test-install

Times in UTC · dashboard · how the merge queue works

@mergify mergify Bot removed the queued label Nov 2, 2026
@codecov

codecov Bot commented Nov 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.07%. Comparing base (3fc614f) to head (f22abb1).
⚠️ Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
src/modules/job-manager/job.c 77.77% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7885      +/-   ##
==========================================
- Coverage   84.08%   84.07%   -0.02%     
==========================================
  Files         607      607              
  Lines      104396   104416      +20     
==========================================
+ Hits        87782    87787       +5     
- Misses      16614    16629      +15     
Files with missing lines Coverage Δ
src/modules/job-manager/jobtap.c 85.01% <100.00%> (+0.12%) ⬆️
src/modules/job-manager/job.c 86.97% <77.77%> (-0.49%) ⬇️

... and 17 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@grondo
grondo deleted the job-manager-dependency-fixes branch November 2, 2026 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants