fix: a failed novostoic run was reported as completed with no results by matthewberry · Pull Request #171 · moleculemaker/mmli-backend

matthewberry · 2026-11-05T22:14:26Z

Stacked on #161 (itself stacked on #160). It relies on the postjob.py check that #161 adds, which fails the Job when error exists. Merge #160 and #161 first.

Problem

The four novostoic commands in chart/values.yaml ended:

…; if [ "$?" -eq 0 ]; then touch ${JOB_OUTPUT_DIR}/success; else touch ${JOB_OUTPUT_DIR}/fail; fi

The command exits 0 either way, and nothing reads fail. A failed run therefore looks like a successful Job:

the watcher marks it completed;
the user gets the "result is ready" email, with a link to a page that has no results;
the results route returns nothing, because novostoicResultPostProcess returns None when there is no output.json.

On prod, 119 of the 670 completed novostoic jobs have fail and no output.json:

64 pathways, 42 dgpredictor and 13 optstoic.
They run from 2025-03-27 to 2026-10-26, with 34 since May.
65 of them have an email address.

I found these by listing each completed job's artifacts with GET /v1/jobs/{id}/artifacts?include=logs.

Change

The four commands now write error instead of fail. fix: a failed CLEAN run was reported as completed with no results #161's postjob.py check uploads the logs, then fails the Job on error, so the job is marked error, as it is for CLEAN, CRISPR-Copies and Mutagenesis. postjob.py is not changed.
The commands still exit 0. Making them exit non-zero would lose output.log and error.log, for the init-container reason given in fix: a failed CLEAN run was reported as completed with no results #161.
A comment above the commands in values.yaml states this rule and names the test that checks it.

The first version of this PR taught postjob.py to also fail on fail. Review pointed out that this applied the new marker to every job type, though only novostoic writes it, so this version switches the commands to the existing marker instead.

Why this is safe

No job with results is turned into an error. None of the 670 completed novostoic jobs has both fail and output.json. Some failed pathways runs leave output.intermediate.json or raw_solution.txt, but the results route reads only output.json.
Nothing reads fail:
- git grep -n 'fail\b' -- app chart finds it only in the four commands and in LOG_ARTIFACTS (app/routers/v1/jobs.py:45), which only hides it from the default artifact listing.
- In novostoic-frontend (default branch, 761404e), grep -rn "'fail'\|\"fail\"\|/fail\b" src finds nothing.
novostoic-frontend handles error: JobResult.statusResponse$ stops polling on any phase other than queued or processing, and the loading component shows its error state on JobStatus.Error.
Deployed config: the deployments mount the chart's configmap over app/cfg/config.yaml (chart/templates/deployment.yaml:81), and no values overlay overrides the novostoic commands. The novostoic commands in app/cfg/config.yaml and app/cfg/local.config.yaml, used only locally, write no marker. That is unchanged.

Testing

New tests/test_novostoic_job_exit_code.py: for each novostoic job type in chart/values.yaml, it renders that command through the real job.tpl.yaml with create_job, then runs the job container's script with a stand-in python.
- Tool exits 1, or is killed with SIGKILL: the container exits 0, and the output directory has error, error.log, finished and output.log.
- Tool succeeds: the container exits 0, and the output directory has success and no error.
- The job type list comes from the chart, and a separate test pins it to the four current types. A fifth novostoic type is therefore covered automatically, and if it lacks the marker its failure test fails.
With the old fail marker restored, all 8 failure cases fail.
Full suite in the Dockerfile.test image: 643 passed.
Not run against the real novostoic image or on the cluster.

Deploy note

This needs a chart deploy, and :main must already have #161's postjob.py check, because post-job always runs moleculemaker/mmli-backend:main. If the chart is deployed first, a failed run is still marked completed, as it is today, so deploying in the wrong order loses nothing. After both are in place:

failed runs are marked error;
users with an email address get the failure email.

Not changed

The failure email has no link. The ERROR branch of send_notification_email (app/services/kubejob_service.py:319) sends "An error occurred in computing the result for your … job." with no link. That is true for every job type, so users can't reach the logs this change keeps. The first version of this description wrongly said the email was the same for every phase.
Pathways runs with no solution: 148 completed pathways jobs have success and no output.json. The one log I read says "No solution file found", and that run exited 0. Whether "no pathway found" should be an empty result or an error is a separate decision.
Past jobs: the 119 jobs already marked completed are left as they are.
The artifact listing: LOG_ARTIFACTS still hides fail, for those past jobs. Hiding error is fix: hide the job template's log and marker files from the v1 artifact listing #163.

The novostoic commands in chart/values.yaml exit 0 and write a `fail` marker when the tool fails, and nothing read that marker. postjob.py now fails the Job on `fail` after the upload, the same way it does on `error`, so the watcher marks the job 'error' and the logs still reach MinIO.

matthewberry

The fail check itself looks correct. I didn't reproduce any of the points below; they come from reading the code at ba3c959.

Checked and cleared

No re-run on failure: backoffLimit: 0 (kubejob_service.py:892) and restartPolicy: Never, so the forced exit 1 doesn't run the novostoic tool again.
git grep for the quoted "fail" and /fail forms over app/ and chart/ finds the marker written only by the four novostoic commands in chart/values.yaml. LOG_ARTIFACTS mentions it but doesn't use it to decide anything.
The watcher's _derive_phase turns a failed pod into error.

Files outside this diff

The PR body is wrong about the email (app/services/kubejob_service.py:319-322). It says send_notification_email builds the same results link for every terminal phase. The ERROR branch sends only "An error occurred in computing the result for your … job." with no results_url, so users can't reach the output.log/error.log this change keeps on purpose.
The in-repo app configs lack the marker. The novostoic commands in app/cfg/config.yaml (~L345) and local.config.yaml don't write fail, so the new comment in postjob.py is true only of chart/values.yaml. Where those configs are used, a failed run exits non-zero in the init container, post-job never runs, and the logs never reach MinIO.
Inconsistent artifact listing (app/routers/v1/jobs.py:45). LOG_ARTIFACTS hides fail but not error, so /v1/jobs/{id}/artifacts lists an empty error file for one marker and hides the other.
Nothing at the write site points to the reader. Nothing in chart/values.yaml, where the commands that write the marker live, says that postjob.py reads the names ('error', 'fail'). The next command that adds or renames a marker will be written there.

matthewberry · 2026-11-05T22:21:04Z

-    log.error(f'Job[{bucket_name}]: {job_id} wrote an error sentinel; failing the Job')
-    sys.exit(1)
+# The novostoic commands in chart/values.yaml write `fail` instead of `error`.
+for sentinel in ('error', 'fail'):


This check now applies to every job type, but only the novostoic commands write fail. Any other tool that leaves a top-level file named fail in JOB_OUTPUT_DIR (its own marker, or a user-named file copied through) would have a successful run marked error and get a failure email. The evidence that no other type does this is a sample of 100 jobs per type. Narrower options: check fail only when JOB_TYPE starts with novostoic-, or change the four commands to touch error so the shared code needs no second marker.

Agreed. I took the second option in 5a1f922: the four commands now write error, and postjob.py is back to its #161 version, so there is no second marker in shared code.

matthewberry · 2026-11-05T22:21:04Z

-if os.path.exists(os.path.join(job_output_dir, 'error')):
-    log.error(f'Job[{bucket_name}]: {job_id} wrote an error sentinel; failing the Job')
-    sys.exit(1)
+# The novostoic commands in chart/values.yaml write `fail` instead of `error`.


The PR body says a search of app/ and chart/ finds touch …/fail only in these four commands, but it doesn't show the search. That pattern also misses other ways of creating the file (> fail, or open('fail', 'w') inside a tool image). Could you put the command next to the claim?

This check is gone from postjob.py now. The description now shows the searches next to the claim: git grep -n 'fail\b' -- app chart, plus a grep of novostoic-frontend's src. The prod scan was of artifact names, so it would catch a fail file however it was created. A tool image that writes one internally is still possible and not checked, but nothing reads fail any more.

matthewberry · 2026-11-05T22:21:04Z

    assert calls == [{"local_path": str(job_env), "bucket_name": "clean", "minio_prefix": "job1/out"}]
+
+
+NOVOSTOIC_TYPES = ["novostoic-dgpredictor", "novostoic-enzrank", "novostoic-optstoic", "novostoic-pathways"]


Because this list is hard-coded, a fifth novostoic job, or one whose command lacks the marker, isn't covered: the test stays green and the original bug comes back for that type. Building it from [k for k in values['config']['kubernetes_jobs'] if k.startswith('novostoic-')] would catch it.

Done in 5a1f922. The new tests/test_novostoic_job_exit_code.py builds the job type list from chart/values.yaml, and test_every_novostoic_type_is_covered pins it, so adding a fifth type also changes a test.

matthewberry · 2026-11-05T22:21:04Z

+    out.mkdir()
+    env = {**os.environ, "PATH": f"{bin_dir}{os.pathsep}{os.environ['PATH']}", "JOB_OUTPUT_DIR": str(out)}
+
+    subprocess.run(["bash", "-c", command], cwd=tmp_path, env=env, check=True)


This runs the bare command, but production wraps it in job.tpl.yaml as ls -al ${JOB_INPUT_DIR} && {{ command }} && ls -al ${JOB_OUTPUT_DIR} && touch finished. In the wrapped form, a failing ls skips python, $? holds ls's exit code, and fail is written with no error.log. A change to the wrapper that alters how $? reaches the if wouldn't be caught either. Rendering the command through the wrapper would test what actually runs.

Done in 5a1f922. The new test renders each command through job.tpl.yaml with create_job and runs the job init container's script, the same way test_clean_job_exit_code.py does. With fail restored, all 8 failure cases fail.

…st-job `fail` Review on #171: checking `fail` in postjob.py applied it to every job type, though only the novostoic commands write it. The four commands now write `error`, which postjob.py already fails the Job on, so postjob.py is back to #161's version. The new test renders each novostoic command from the chart through the real job template, and builds the job type list from the chart.

matthewberry · 2026-11-05T22:24:57Z

Thanks for the review. Changes are in 5a1f922, and the description is rewritten.

Email: you're right. The ERROR branch sends no link. I read only the code that builds the link, not the send below it. I corrected the description and listed the missing link under "Not changed", since every job type behaves this way.
fail in shared code: the commands now write error, so postjob.py is unchanged from fix: a failed CLEAN run was reported as completed with no results #161.
Tests: the job type list now comes from the chart, and the test runs each command through the job.tpl.yaml wrapper.
The other notes:
- In-repo configs: deployments mount the chart's configmap over app/cfg/config.yaml (chart/templates/deployment.yaml:81), so config.yaml and local.config.yaml are used only locally. I left them alone and said so in the description.
- LOG_ARTIFACTS: hiding error is fix: hide the job template's log and marker files from the v1 artifact listing #163. fail stays hidden for the past jobs that have it.
- The write site: a comment above the novostoic commands in values.yaml now states the rule and names the test.

matthewberry commented Nov 5, 2026

View reviewed changes

matthewberry requested a review from bodom0015 November 5, 2026 22:33

This was referenced Nov 5, 2026

fix: keep failed CRISPR-Copies and Mutagenesis logs by failing the Job in post-job #169

Open

fix: a failed CLEAN run was reported as completed with no results #161

Open

Navigation Menu

Repository navigation

fix: a failed novostoic run was reported as completed with no results - #171

fix: a failed novostoic run was reported as completed with no results#171
matthewberry wants to merge 2 commits into
mjberry/fix/clean-pipefailfrom
mjberry/fix/novostoic-fail-marker

matthewberry commented Nov 5, 2026 •

edited

Loading

Uh oh!

matthewberry left a comment

Uh oh!

matthewberry Nov 5, 2026

Uh oh!

matthewberry Nov 5, 2026

Uh oh!

matthewberry Nov 5, 2026

Uh oh!

matthewberry Nov 5, 2026

Uh oh!

matthewberry Nov 5, 2026

Uh oh!

matthewberry Nov 5, 2026

Uh oh!

matthewberry Nov 5, 2026

Uh oh!

matthewberry Nov 5, 2026

Uh oh!

matthewberry commented Nov 5, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		assert calls == [{"local_path": str(job_env), "bucket_name": "clean", "minio_prefix": "job1/out"}]


		NOVOSTOIC_TYPES = ["novostoic-dgpredictor", "novostoic-enzrank", "novostoic-optstoic", "novostoic-pathways"]

Conversation

matthewberry commented Nov 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Change

Why this is safe

Testing

Deploy note

Not changed

Uh oh!

matthewberry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewberry commented Nov 5, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

matthewberry commented Nov 5, 2026 •

edited

Loading