Skip to content

Conversation

@wvangeit
Copy link
Contributor

What do these changes do?

This is part of #8280

The idea is to avoid return 'results' unless a computational project/solver job has succeeded, not having this can lead to returning default/out-of-date results.
The function api already protects against this, but I now added it on the level of solver and study jobs.

Related issue/s

closes #8510

How to test

Check if a failed job returns returns results.

Dev-ops

No changes

@wvangeit wvangeit self-assigned this Oct 14, 2025
@wvangeit wvangeit added the a:apiserver api-server service label Oct 14, 2025
@wvangeit wvangeit requested a review from sanderegg October 14, 2025 09:01
@wvangeit wvangeit marked this pull request as ready for review October 14, 2025 09:01
@wvangeit
Copy link
Contributor Author

@sanderegg if you could check carefully if this makes sense. Imo we should not return an output if the job is not done, but maybe you're aware of some tools depending on this?

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

thx. I highly recommend adding test coverage on these error scenarios

@codecov
Copy link

codecov bot commented Oct 14, 2025

Codecov Report

❌ Patch coverage is 68.00000% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.95%. Comparing base (6213a3d) to head (6991c0d).
⚠️ Report is 1 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (6213a3d) and HEAD (6991c0d). Click for more details.

HEAD has 30 uploads less than BASE
Flag BASE (6213a3d) HEAD (6991c0d)
unittests 31 1
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #8511       +/-   ##
===========================================
- Coverage   86.28%   68.95%   -17.33%     
===========================================
  Files        1931      905     -1026     
  Lines       75777    40157    -35620     
  Branches     1343      175     -1168     
===========================================
- Hits        65382    27690    -37692     
- Misses      10007    12410     +2403     
+ Partials      388       57      -331     
Flag Coverage Δ
integrationtests 64.02% <ø> (+4.34%) ⬆️
unittests 91.76% <68.00%> (+5.83%) ⬆️
Components Coverage Δ
pkg_aws_library ∅ <ø> (∅)
pkg_celery_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library ∅ <ø> (∅)
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 76.81% <ø> (+11.62%) ⬆️
agent ∅ <ø> (∅)
api_server 91.76% <68.00%> (-0.10%) ⬇️
autoscaling ∅ <ø> (∅)
catalog ∅ <ø> (∅)
clusters_keeper ∅ <ø> (∅)
dask_sidecar ∅ <ø> (∅)
datcore_adapter ∅ <ø> (∅)
director ∅ <ø> (∅)
director_v2 78.01% <ø> (-7.34%) ⬇️
dynamic_scheduler ∅ <ø> (∅)
dynamic_sidecar 81.89% <ø> (-8.55%) ⬇️
efs_guardian ∅ <ø> (∅)
invitations ∅ <ø> (∅)
payments ∅ <ø> (∅)
resource_usage_tracker ∅ <ø> (∅)
storage ∅ <ø> (∅)
webclient ∅ <ø> (∅)
webserver 59.04% <ø> (-28.08%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6213a3d...6991c0d. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mergify
Copy link
Contributor

mergify bot commented Oct 14, 2025

🧪 CI Insights

Here's what we observed from your CI run for 6991c0d.

✅ Passed Jobs With Interesting Signals

Pipeline Job Signal Health on master Retries 🔍 CI Insights 📄 Logs
CI system-tests Base branch is broken, but the job passed. Looks like this might be a real fix 💪 Broken 0 View View
unit-tests Base branch is broken, but the job passed. Looks like this might be a real fix 💪 Broken 0 View View

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

seems good

Copy link
Contributor

@bisgaard-itis bisgaard-itis left a comment

Choose a reason for hiding this comment

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

Cool, thanks!

@wvangeit wvangeit enabled auto-merge (squash) October 15, 2025 14:03
@wvangeit
Copy link
Contributor Author

@mergify queue

@mergify
Copy link
Contributor

mergify bot commented Oct 15, 2025

queue

🟠 Waiting for conditions to match

  • -closed [📌 queue requirement]
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • label=🤖-automerge
      • #approved-reviews-by >= 2 [🛡 GitHub branch protection]
      • #approved-reviews-by>=2
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by=0
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • #review-threads-unresolved=0
      • -conflict
      • -draft
      • base=master
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • label!=🤖-do-not-merge
      • any of: [🛡 GitHub branch protection]
        • check-skipped = deploy to dockerhub
        • check-neutral = deploy to dockerhub
        • check-success = deploy to dockerhub
      • any of: [🛡 GitHub branch protection]
        • check-success = system-tests
        • check-neutral = system-tests
        • check-skipped = system-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = unit-tests
        • check-neutral = unit-tests
        • check-skipped = unit-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = check OAS' are up to date
        • check-neutral = check OAS' are up to date
        • check-skipped = check OAS' are up to date
      • any of: [🛡 GitHub branch protection]
        • check-success = integration-tests
        • check-neutral = integration-tests
        • check-skipped = integration-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = build-test-images (frontend) / build-test-images
        • check-neutral = build-test-images (frontend) / build-test-images
        • check-skipped = build-test-images (frontend) / build-test-images
      • any of: [🛡 GitHub branch protection]
        • check-success = SonarCloud Code Analysis
        • check-neutral = SonarCloud Code Analysis
        • check-skipped = SonarCloud Code Analysis
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed

@wvangeit
Copy link
Contributor Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Oct 15, 2025

update

☑️ Nothing to do, the required conditions are not met

  • #commits-behind > 0 [📌 update requirement]
  • -closed [📌 update requirement]
  • -conflict [📌 update requirement]
  • queue-position = -1 [📌 update requirement]

@sonarqubecloud
Copy link

@wvangeit wvangeit merged commit 48dfd7c into ITISFoundation:master Oct 15, 2025
91 of 95 checks passed
@wvangeit wvangeit added this to the Imparable milestone Oct 15, 2025
bisgaard-itis pushed a commit to bisgaard-itis/osparc-simcore that referenced this pull request Oct 15, 2025
bisgaard-itis pushed a commit to bisgaard-itis/osparc-simcore that referenced this pull request Oct 15, 2025
bisgaard-itis pushed a commit to bisgaard-itis/osparc-simcore that referenced this pull request Oct 15, 2025
bisgaard-itis pushed a commit to bisgaard-itis/osparc-simcore that referenced this pull request Oct 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a:apiserver api-server service

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make sure study job API doesnt return outputs if job is not SUCCEEDED

4 participants