[matthewberry]

Closes #158.

Problem

The backend reports a failed job with phase 'error' (see JobStatus in the generated client), but the results page checked for 'failed'. When a job failed, the page kept polling GET /clean/jobs/{id} every 10 seconds and never showed the failure message.

Change

• In results.component.ts, the two status checks (stop polling, set failedJob) now compare against JobStatus.Error instead of 'failed'. The 'completed' check now uses JobStatus.Completed. Completed jobs are handled the same as before.
• PollingResponseStatus.status is now typed JobStatus instead of string. The results component is the only code that uses this type. With this type, a comparison against 'failed' doesn't compile (TS2367).

Testing

Added tests in results.component.spec.ts. They use a stubbed ResultService whose polling stream ends when gotEndResult() is called, like the real service:

• error job: polling stops, failedJob is set, results aren't fetched, and .failJob renders
• completed job: polling stops, results are fetched, and there's no failure message
• processing job: polling continues

npx ng test --watch=false --browsers=ChromeHeadless: 9 of 9 pass on this branch, which is rebased onto main after #160. With the old 'failed' checks restored, the error-job test fails. ng build succeeds. I didn't test against a real failed job in a browser.

[matthewberry]

Review follow-up, pushed as 0554a39. Each fix below has a spec that failed before the change and passes after it. Full suite: 17/17 passing.

Reproduced and fixed

• Status errors never reached the UI. retry({ delay: 10000 }) had no count limit, so a 404/403/500 from the status endpoint kept the loader spinning forever. Retries are now capped at 5 consecutive failures (resetOnSuccess: true, so occasional errors during a long job don't add up). The component now shows the failure message when polling errors.
• Missing job_info threw on every poll. For a queued or processing job, JSON.parse(job_info!) threw inside the next handler every 10s. It is now skipped when job_info is absent. The existing spec "stops polling for an errored job even when job_info is missing" now expects no unhandled error.
• Unknown job id polled forever. When the job lookup returns [], polling now stops and the failure message is shown.
• Job id read from window.location. The component now reads it from ActivatedRoute.paramMap and reloads when the id changes. The router reuses the component between /results/A and /results/B, and before this change ngOnInit never ran again for the second job. That navigation can't happen in the UI today, because the only links to /results/:id come from the configuration page. The new spec guards it anyway. This change also means the specs no longer depend on the Karma page URL.

Checked, not a bug

• The concern that polling continues after leaving the page: ngOnDestroy already unsubscribes. A spec now guards that.

Not addressed

• status: firstMatch?.phase! still hides that Job.phase is optional. A job with no phase keeps polling.
• For an errored job with no job_info, the template still shows "Valid No. of Sequences: undefined Sequence(s)".

Open questions

• Can the backend briefly return [] for a job that was just submitted? If so, treating [] as "not found" will wrongly fail real jobs, and it should allow a few empty responses first.
• Is a cap of 5 retries (about 50s) right?
• /results with no id now passes undefined as the job id. Previously it polled for a job named results. Either way the page ends up showing the failure message.

[matthewberry]

Follow-up on status: firstMatch?.phase!, pushed as e31ed41.

The backend (mmli-backend origin/main at 091482a) shows the ! was hiding a different problem than a missing phase:

• job.phase is NOT NULL with a default of queued (app/models/sqlmodel/models.py), so the job list endpoint should never return a job without a phase. The client marks it optional only because the field has a default.
• The backend's JobStatus (app/models/enums.py) also has canceled, which the generated client enum (queued | processing | completed | error) lacks. A canceled job matched neither terminal check, so the page polled every 10s forever.

Fix: any phase other than queued or processing now ends polling. The page shows the failure message unless the job completed. That covers canceled, any status added later, and a missing phase. PollingResponseStatus.status is now optional to match Job.phase, so the ! is gone. The completion handler no longer re-derives failedJob; the next handler sets it before stopping polling.

New specs cover a job with no phase and a canceled job. Both failed before the fix. Full suite: 19/19 passing, and ng build succeeds.

Two related notes:

• A canceled job now shows the generic "Job failed possibly due to incorrect input…" message. A dedicated "canceled" message would need a product decision.
• Regenerating the client from the current backend OpenAPI spec would add Canceled to the enum.

From the same backend read: for an unknown id, the list endpoint returns [] rather than a 404 (app/routers/job.py:232), which confirms the not-found path fixed in 0554a39 is reachable. I didn't check whether the backend commits the job row before the create request returns, so the earlier question about a brief [] right after submission is still open.

[matthewberry]

Review follow-up, pushed as d111b3e.

Reproduced and fixed

• A completed job whose results fail to load spun forever. getResponseResult()'s error handler only logged, so contentLoaded stayed false and the loader never cleared. The backend's results endpoint answers 404 for a completed CLEAN job with no _maxsep.csv (cleanResultPostProcess, app/services/clean_service.py). Jobs like that already exist: crashed runs that were marked completed before fix: a failed CLEAN run was reported as completed with no results mmli-backend#161, such as f289fd27. The handler now sets failedJob and contentLoaded, as the status-polling error handler already does.
• New spec "shows the failure message when a completed job has no results": it fails without the fix (1 of 20), and the full suite passes with it (20 of 20, ChromeHeadless). ng build succeeds; its only warning is the existing app.component.scss budget.

Answers the open question from the first follow-up

• Can the backend briefly return [] for a just-submitted job? No. create_job runs db.add and await db.commit() (app/routers/job.py:93–94 on mmli-backend origin/main) before it returns, and the configuration page navigates to /results/:id only from that response's next handler. So treating [] as "not found" can't fail a real job. The example path never polls: price149 is the only example, and loadJob() special-cases it.

Still not addressed (unchanged)

• Navigating from one job's results to another doesn't reset statusResponse or numOfSeq, so the header shows the old job's values until the first poll. Nothing in the UI links between results pages today.
• A canceled job shows the generic failure message.

Merge order: this should be deployed before mmli-backend#161. With that change, crashed CLEAN runs are reported as error, which main polls forever.

[matthewberry]

A backend outage of about a minute now permanently fails a job that is still running. Six status calls failing in a row (the first try plus 5 retries, 10s apart) sends the error to ResultsComponent, which sets failedJob and shows "Job failed…". Nothing polls again, so the user never sees the results unless they reload. Before this change, retry() kept polling until the backend came back.

This test reproduces it (it fails with Expected spy onError not to have been called.):

it('keeps polling through a one-minute backend outage', fakeAsync(() => {
  const processing = [{ job_id: 'abc', phase: JobStatus.Processing }];
  let calls = 0;
  spyOn(TestBed.inject(JobsService), 'listJobsByTypeAndJobIdJobTypeJobsJobIdGet')
    .and.callFake((() => ++calls <= 6 ? throwError(() => new Error('503')) : of(processing)) as any);
  const onNext = jasmine.createSpy('onNext');
  const onError = jasmine.createSpy('onError');
  const subscription = service.getResult('abc').subscribe({ next: onNext, error: onError });

  tick(2 * 60 * 1000);

  expect(onError).not.toHaveBeenCalled();
  expect(onNext).toHaveBeenCalledWith(processing);

  subscription.unsubscribe();
  discardPeriodicTasks();
}));

If the goal is to stop polling a job that will never be found (e.g. a 404), could the retry be limited to that case, or allowed to run much longer than a minute? surfaces the error to the subscriber when status requests keep failing still passes as long as there's some limit under 10 minutes.

[matthewberry]

Agreed, fixed in a448e6c.

The cap only bought us something for errors that won't clear up, and the status endpoint has almost none. On mmli-backend origin/main, list_jobs_by_type_and_job_id (app/routers/job.py) has no auth dependency, and nothing in main.py can reject the request. Its only 4xx is a 400 for an invalid job type, and this page hardcodes 'clean'. An unknown id is a 200 with [], which the component already treats as not found. What's left is network errors and 5xx during outages, which clear up on their own.

So retry now retries network errors and 5xx every 10s for as long as they last, which is the behavior before this PR. A 4xx (HttpErrorResponse with status 400–499) ends polling right away, and the page shows the failure message.

Specs in result.service.spec.ts:

• Your outage case, made longer: 60 failed calls (about ten minutes) followed by a processing response, run once with 503s and once with network errors (status 0). Neither errors, and polling resumes.
• A 404 surfaces right away with no retry.
• The existing "waits before retrying" spec now uses an HttpErrorResponse 500.

Without the change, those three new specs fail. With it, 22 of 22 pass, and ng build succeeds.

[matthewberry]

This treats a missing phase as final and failed. Job.phase is optional in the generated model, so if jobmgr can ever return a row before its phase is written, the job is marked failed and polling stops for good, even though the job then runs normally.

This test reproduces it (polling has stopped, failedJob is true, results are never fetched, and .failJob is showing):

it('keeps polling when a poll returns the job without a phase', () => {
  polls.next([{ ...job(JobStatus.Processing), phase: undefined }]);
  fixture.detectChanges();

  expect(polls.observed).toBeTrue();
  expect(fixture.componentInstance.failedJob).toBeFalse();

  polls.next([job(JobStatus.Completed)]);
  fixture.detectChanges();

  expect(fixture.componentInstance.failedJob).toBeFalse();
  expect(resultService.getResponse).toHaveBeenCalled();
  expect(fixture.nativeElement.querySelector('.failJob')).toBeNull();
});

It directly contradicts stops polling and shows the failure message when the job has no phase, so the question is whether the backend can actually return a job without a phase. If it can't, ignore this. If it can, I'd keep polling when phase is missing and treat only known final phases (completed, error, canceled) as final.

[matthewberry]

Checked, and the backend can't return a job without a phase, so I've left this as is.

• The column is never null: phase: JobStatus = Field(default=JobStatus.QUEUED, nullable=False, …) in app/models/sqlmodel/models.py (mmli-backend origin/main).
• The field is never dropped: list_jobs_by_type_and_job_id returns the ORM rows directly, with no response_model or exclude_none that could leave phase out. The only optional phase in that file (line 178) is on a separate model, not the one this route returns.
• No new phase is coming: open mmli-backend#153 (the queued-until-the-container-starts change) sets the existing queued. It doesn't add a phase value, so nothing new is on the way that this check would wrongly treat as final.

The missing-phase spec is a guard against a response that can't happen today. If the backend ever adds a phase that isn't final, this check would show "Job failed" for a job that's still running. The JobStatus enum is where that phase would be added, and the generated client would need regenerating then anyway.

[matthewberry]

Confirmed on mmli-backend: phase is nullable=False with a default of queued (app/models/sqlmodel/models.py:100). The missing-phase case can't happen, so the existing spec is right and mine was wrong. Resolving from my side.

[matthewberry]

Only an HttpErrorResponse with a 4xx status ends polling, so any other error is retried forever. One of those is reachable: on the bare /results route (app-routing.module.ts:11) there's no :id, and the generated client throws a plain Error('Required parameter jobId was null or undefined…') before any request is sent. At d111b3e the 5-retry cap showed the failure message after about a minute. Now the loader spins forever.

This test fails at a448e6c with Expected spy onError to have been called. (it uses the real JobsService, no spy):

it('surfaces the error when there is no job id (the bare /results route)', fakeAsync(() => {
  const onError = jasmine.createSpy('onError');
  const subscription = service.getResult(undefined).subscribe({ error: onError });

  tick(10 * 60 * 1000);
  expect(onError).toHaveBeenCalled();

  subscription.unsubscribe();
  discardPeriodicTasks();
}));

Low severity: you only get there by typing the URL, and main also polled forever. Either show the failure state before polling when there's no id, or retry only HttpErrorResponses with status 0 or 5xx and treat everything else as final.

[matthewberry]

Confirmed and fixed. I took your second option: only failures that can clear up are retried, and everything else is final.

• b751745: retry only an HttpErrorResponse with status 0 or 5xx. Anything else ends polling and shows the failure message, including the plain Error the generated client throws for a null or undefined jobId (jobs.service.ts:337–338). Your spec is in result.service.spec.ts, with the wait shortened from 10 minutes to tick(1), since the error now arrives right away. It fails at a448e6c and passes now.
• d866192: 408 and 429 are also retried. A proxy or ingress in front of the backend can send them while the job is still running. This came up in review on iBioFoundry/crispr-copies-frontend#19, which shares this logic. The outage spec now covers 503, 0, 408 and 429, and the 408 and 429 cases fail at b751745.

25 of 25 pass, and ng build succeeds.