[Improve][Zeta] Mark unknown job-info responses with found:false by wgzhao · Pull Request #12904 · apache/seatunnel

wgzhao · 2026-10-19T14:10:22Z

Yes, I am contributing this change to Apache SeaTunnel.

Purpose of this change

GET /job-info/{jobId} returned HTTP 200 with only jobId when the id was neither running nor
retained in finished-job history. Clients could not distinguish that from a successful lookup,
and a status-code-based check treated a missing job as healthy.

Per the compatibility proposal accepted in #12785, this PR keeps HTTP 200 and successful
payloads unchanged, and adds an explicit found: false marker for unknown or expired ids.

Changes

Server: JobInfoService adds found: false to the unknown-id response; running and finished
payloads are unchanged. Adds RestConstant.FOUND.
Web UI: detects found: false (and the status-less response from older servers), shows a
missing/expired-job state, and stops polling before parsing timestamps.
Python SDK: documents the response shape and adds job_exists() with a jobStatus fallback
for older servers.
CLI: --diagnose prefers the explicit marker and keeps the status-less fallback; post-submit
polling stops with a clear message instead of swallowing an absent job.
EN/ZH REST API docs, job lifecycle troubleshooting, and Python SDK docs updated.
/jobs/checkpoints/{jobId} is intentionally out of scope.

Compatibility

Additive response field; HTTP status and successful payloads are unchanged.
Older engines omit found; clients fall back to the presence of jobStatus.

Release note

/job-info/{jobId} now returns {"jobId": "...", "found": false} for unknown or expired ids.
Existing successful responses are unchanged; clients should check found (or jobStatus when
talking to older engines).

Review follow-up

Added a server test for the running-job branch, asserting that a running payload does not gain
the found marker.
Documented the marker in the EN/ZH v1 pages and in the deprecated /running-job/:jobId blocks
of v2, all of which share the same service.
CLI polling now tolerates one status-less 200 before declaring the job unknown, covering the
master-failover window on older servers. An explicit found: false still stops immediately.
The post-submit message no longer suggests a typo, and a repeated status-less response uses a
distinct response-shape/failover message.
The Web UI now renders a load-error result when getJobInfo rejects, and the not-found card
shows the requested job id. It also tolerates one status-less response during an old-server
failover before switching to the missing state, mirroring the CLI.
After a successful load, a transient refresh failure keeps the populated page, shows a single
warning for that error streak, and retries on the next 5 s tick; the full error card is only
used before the first successful load. The warning state is reset on the next successful
refresh. A useMessage().warning spy now pins one warning per error streak, deduplicates an
identical consecutive error, warns again for a changed error, and resets after recovery.
JobsApi.job_exists raises TypeError for a non-JSON body instead of reporting the job as
absent.
Rebased onto the latest dev (bab2d089c).

Tests

Server: JobInfoServiceNullSafetyTest — 8 tests (finished and running payloads omit found,
unknown id returns found: false).
UI: detail.spec.ts — 7 tests, including found: false, the status-less old-server response,
recovery after one status-less response, polling stop, an initial rejection, a rejection after
a successful load, and warning dedupe/reset.
Python SDK: test_sdk.py — 11 tests, including job_exists for new/old shapes and a non-JSON
response.
CLI: test_diagnose_cli.py — 29 tests, including the found marker, polling stop, and recovery
after one status-less response.
spotless:check and the engine-server verify pass; the UI vue-tsc type-check passes.

CI note

The current Build failure in LLMBooleanOutputTest is a base issue: this PR does not change
seatunnel-transforms-v2, and the same InvalidClassException ... AbstractMultiCatalogTransform
failure reproduces locally on the rebased head. The FakeSourceToAssertIT failure is an unrelated
E2E flake.

SEZ9

Thanks for working on this. I reviewed the latest head 54944fa7658f.

What this PR fixes

User pain: GET /job-info/{jobId} for an id that is neither in IMAP_RUNNING_JOB_INFO nor in finished-job history answered HTTP 200 with only {"jobId": ...}, so the Web UI parsed undefined timestamps and kept polling, the CLI --diagnose read it as "Nothing to report", and status-code-based checks treated a missing job as healthy.
Fix approach: keep HTTP 200 and the running/finished payloads unchanged, but make the fallthrough branch of JobInfoService.getJobInfoJson(Long) (JobInfoService.java:114-116) emit .add(RestConstant.FOUND, false); then teach the three consumers (detail.tsx:94 if (res.found === false || !res.jobStatus), cli.py _poll_job_status/run_diagnose, and JobsApi.job_exists) to prefer the marker and fall back to "no jobStatus" for older engines, with unit tests and v2 docs for each.
One-line summary: the server change is minimal and correct and the consumers are consistent and tested; the main gap is documentation — JobInfoService.getJobInfoJson is wired to both REST_URL_JOB_INFO and REST_URL_RUNNING_JOB in JettyService.java:264-265 and to the v1 /hazelcast/rest/maps/{job-info,running-job} handler in RestHttpGetCommandProcessor.java:146-148/271, yet rest-api-v1.md (en/zh) and the deprecated /running-job section of rest-api-v2.md (en/zh) still show {"jobId" : ""}. The remaining items are CLI/UI robustness nits: post-submit polling now gives up on the first non-dict or status-less body with a "the id may be wrong" message for an id the CLI itself just received, the UI fetch still has no rejection path, and the SDK's job_exists reports a non-JSON body as "job does not exist".

Runtime chain I rechecked

Jetty v2 (JettyService.java:264-265 maps both REST_URL_JOB_INFO and REST_URL_RUNNING_JOB to the same jobInfoHolder)
  JobInfoServlet.doGet()
    -> JobInfoService.getJobInfoJson(Long jobId)  JobInfoService.java:77
         running: IMAP_RUNNING_JOB_INFO hit -> convertToJson(jobInfo, jobId)  (unchanged)
         finished: IMAP_FINISHED_JOB_STATE hit -> BaseService.getJobInfoJson(state, metrics, dag)  JobInfoService.java:111-112 (unchanged)
         unknown:  new JsonObject().add(JOB_ID, ...).add(RestConstant.FOUND, false)  JobInfoService.java:114-116 (CHANGED)
    -> BaseServlet.writeJson(resp, json)  (HTTP 200)
Hazelcast v1 /hazelcast/rest/maps/{job-info,running-job}/{id}
  RestHttpGetCommandProcessor  :146-148 routes both prefixes -> handleJobInfoById()  :267
    -> jobInfoService.getJobInfoJson(Long.valueOf(jobId))  :271  (same branch, so v1 also returns found:false)

Consumers:
  UI detail.tsx fetch()  detail.tsx:92-116
    -> await getJobInfo(jobId)  :93 (no try/catch)
    -> if (res.found === false || !res.jobStatus) { missing=true; clearInterval(timer); clearTimeout(fetchTimer); pinStore.clear(); return }  :94-99
    -> else parse(res.createTime) / setTimeout(fetch, 5000)  :103-110
    -> render NResult t('detail.notFound.title') / t('detail.notFound.description')  :571-581
  CLI seatunnel_cli/cli.py
    _run_via_rest_api(): POST /submit-job -> job_id = body.get("jobId")  :1346 -> _poll_job_status(api_base, str(job_id))  :1358
    _poll_job_status(): rest.request_json(job-info) -> not dict | found is False | jobStatus missing/blank -> print 'not known to the cluster ... The id may be wrong' ; return  :1374-1385 ; transport exceptions still swallowed by except Exception: pass  :1395-1396
    run_diagnose(): job_info.get('found') is False -> 'marked it as unknown'; return 1  :1676-1683 ; then the existing status-less fallback  :1685-1693
  Python SDK tools/seatunnel-python-sdk/seatunnel/endpoints/jobs.py
    Client.request(): resp.json() only when Content-Type contains application/json, else resp.text  client.py:66-67
    JobsApi.job_exists(): non-dict -> False  jobs.py:99-100 ; 'found' in details -> bool(found)  :101-102 ; else 'jobStatus' in details  :103

Findings

Issue 1: The new found: false response is only documented for /job-info/:jobId in rest-api-v2.md; the same payload is also served on the deprecated /running-job/:jobId (v2) and on both v1 endpoints, whose docs still show {"jobId": ""}

Location: docs/en/engines/zeta/rest-api-v2.md:691
Why it matters: - Root cause: JobInfoService.getJobInfoJson(Long) is the single producer of this payload. JettyService.java:264-265 registers the same jobInfoHolder for both REST_URL_JOB_INFO and REST_URL_RUNNING_JOB, and the v1 (Hazelcast) dispatcher at RestHttpGetCommandProcessor.java:146-148 routes both /running-job and /job-info prefixes to handleJobInfoById, which calls jobInfoService.getJobInfoJson(...) at line 271. After this PR all four endpoints therefore return {"jobId": "<id>", "found": false} for an unknown id.
Only the /job-info/:jobId block of docs/en/engines/zeta/rest-api-v2.md (lines 686-697) and its ZH twin were updated. The deprecated /running-job/:jobId block in the same EN file (lines 770-780) still says the core fields "always be returned" and shows {"jobId" : ""}; the ZH v2 file repeats that at lines 742-752; docs/en/engines/zeta/rest-api-v1.md keeps the old example at lines 366-372 and ~456-460 (with "always be returned" at 339 and 450), and docs/zh/engines/zeta/rest-api-v1.md at lines 388 and 454.
Impact: a reader of the v1 page or of the /running-job section has no way to learn about the new key or the jobStatus compatibility fallback, and the pages now contradict the server. Documentation-only; no runtime effect.
Suggested fix: copy the new example and the one-paragraph explanation into the three remaining blocks (EN/ZH v1 /job-info and /running-job, EN/ZH v2 /running-job), or add a single sentence to each pointing at the /job-info description.
Evidence: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/rest/service/JobInfoService.java:116 .add(RestConstant.FOUND, false);; docs/en/engines/zeta/rest-api-v2.md:686 An unknown id is still an HTTP 200 response. It contains the requested id and an explicit; docs/en/engines/zeta/rest-api-v2.md:691 "found" : false
Suggested fix: This is the only place the new marker is documented, but JobInfoService.getJobInfoJson(Long) also backs the deprecated /running-job/:jobId servlet (JettyService.java:264-265) and both v1 endpoints (RestHttpGetCommandProcessor.java:146-148, :271). The /running-job/:jobId block further down this file (lines 774-780), its ZH twin (docs/zh/engines/zeta/rest-api-v2.md:746-752), and docs/{en,zh}/engines/zeta/rest-api-v1.md (/job-info and /running-job) still show {"jobId" : ""} and say the core fields "always be returned". Could you update those blocks with the same found: false example and explanation so the other pages do not contradict the server?
Severity: Medium

Issue 2: CLI post-submit polling now aborts on the first status-less / non-dict body, turning a single odd response into a permanent 'job not known' verdict

Location: seatunnel-cli/seatunnel_cli/cli.py:1385
Why it matters: - Root cause: before this change _poll_job_status tolerated odd bodies for the whole 5-minute window: a non-dict body raised AttributeError on .get inside the try and was swallowed by the existing except Exception: pass (cli.py:1395-1396), and a dict without jobStatus printed UNKNOWN and kept polling. The new guard at cli.py:1374-1379 returns on the very first response that is not a dict, carries found: false, or lacks a non-blank jobStatus.
The same loop still treats a 5xx, a timeout or a connection reset as retryable, so the CLI is now stricter about one malformed 200 body than about a hard transport failure. One null/list body from a proxy, or one status-less 200, ends polling with "The id may be wrong" seconds after the CLI printed the id the engine allocated (cli.py:1348).
Impact: low-frequency but confusing — the user is told the job is unknown while it is still running, and the terminal-state summary from _show_job_result is never shown.
Suggested fix: keep found: false as the signal, but only give up after it repeats for a few consecutive polls (or after a short grace window when no status has yet been seen), and keep treating non-dict bodies as retryable, matching the exception path. The new parametrized test (test_polling_stops_when_the_job_is_not_known) asserts calls["count"] == 1, so it would need to change accordingly.
Evidence: seatunnel-cli/seatunnel_cli/cli.py:1375 not isinstance(info, dict); seatunnel-cli/seatunnel_cli/cli.py:1376 or info.get("found") is False; seatunnel-cli/seatunnel_cli/cli.py:1377 or not isinstance(info.get("jobStatus"), str)
Suggested fix: This return makes a single odd response terminal. Before this change a non-dict body raised inside the try and was swallowed by the except Exception: pass below (so polling continued), and a dict without jobStatus printed UNKNOWN and kept going. Now one null/list body from a proxy, or one status-less 200, ends polling with "The id may be wrong" seconds after we printed the id the engine allocated. I'd keep found: false as the signal but only give up after it repeats for a few consecutive polls (or after a short grace window when no status has been seen yet), and keep treating non-dict bodies as retryable like the exception path does.
Severity: Medium

Issue 3: found: false is also emitted by the v1 /hazelcast/rest/maps/job-info/:jobId and deprecated /running-job/:jobId endpoints, but only the v2 /job-info docs were updated

Location: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/rest/service/JobInfoService.java:116
Why it matters: - Root cause: the marker is added in JobInfoService.getJobInfoJson(Long) (JobInfoService.java:116), which is shared by the v2 servlet registered for both REST_URL_JOB_INFO and REST_URL_RUNNING_JOB (JettyService.java:264-265) and by the v1 handler RestHttpGetCommandProcessor.handleJobInfoById (RestHttpGetCommandProcessor.java:267-271), which the dispatcher reaches for both job-info/:jobId and the deprecated running-job/:jobId prefixes (:146-148).
docs/en/engines/zeta/rest-api-v1.md (lines 366-372 and the running-job block around line 456-460, with "always be returned" at 339 and 450) and docs/zh/engines/zeta/rest-api-v1.md (lines 388 and 454) still document the unknown-id response as {"jobId" : ""}.
Impact: v1 consumers are not told about the new key or the jobStatus fallback, and the v1 page now contradicts what the server sends. Docs only.
Suggested fix: mirror the v2 wording in both v1 files, or add a note in v2 that the v1 endpoints share this response.
Evidence: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/rest/service/JobInfoService.java:116 .add(RestConstant.FOUND, false);; docs/en/engines/zeta/rest-api-v2.md:686 An unknown id is still an HTTP 200 response. It contains the requested id and an explicit
Suggested fix: This method is also the backend for the v1 RestHttpGetCommandProcessor.handleJobInfoById (/hazelcast/rest/maps/job-info/:jobId and the deprecated /running-job/:jobId), so those endpoints now return found: false too. docs/en|zh/engines/zeta/rest-api-v1.md still show {"jobId" : ""} for the unknown case and say the core fields "always be returned". Could you mirror the v2 wording there (or note in v2 that v1 shares this response) so v1 consumers know about the new key and the jobStatus fallback?
Severity: Medium

Issue 4: CLI post-submit polling terminates on the first ambiguous body and reports a non-object payload as "job is not known to the cluster"

Location: seatunnel-cli/seatunnel_cli/cli.py:1375
Why it matters: - Root cause: the new guard in _poll_job_status (cli.py:1374-1379) folds three different situations into one early return: the definitive found is False marker, the status-less dict from an older server, and not isinstance(info, dict) (a bare null, a list, or similar body from a proxy). All three print the same "Job ... is not known to the cluster" message (cli.py:1380-1385).
The normal path is safe: CoordinatorService.submitJob does runningJobInfoIMap.put(jobId, submittedJobInfo) (CoordinatorService.java:1454) before jobSubmitFuture.complete(null) (:1463), and the REST submit path join()s that future (BaseService.java:1396-1401), so the first poll after a successful submit finds the job. The problem is only the non-object branch: a non-object body is a response-shape problem, not evidence that the job is gone, yet the message blames the id.
run_diagnose keeps two separate branches for these cases (cli.py:1676-1683 for found: false, :1685-1693 for status-less/non-dict), so the polling path is now less precise than the diagnose path.
Suggested fix: stop immediately only for found is False; for the non-dict / status-less cases print a distinct message, or allow a couple of consecutive misses before giving up.
Evidence: seatunnel-cli/seatunnel_cli/cli.py:1376 or info.get("found") is False; seatunnel-cli/seatunnel_cli/cli.py:1381 f" [error]Job {job_id} is not known to the cluster.[/error] "
Suggested fix: not isinstance(info, dict) ends the poll with "Job ... is not known to the cluster", but a non-object body is a response-shape problem (proxy, bare null), not evidence that the job is gone. The loop already keeps retrying transport exceptions for 5 minutes, so one malformed 200 body shouldn't be fatal with that message. Suggest: stop immediately only for found is False; for the non-dict / status-less cases print a distinct message or allow a couple of consecutive misses before giving up, like run_diagnose does with its two separate branches.
Severity: Low

Issue 5: Post-submit polling message says "The id may be wrong" for an id the CLI itself just received from /submit-job

Location: seatunnel-cli/seatunnel_cli/cli.py:1382
Why it matters: - Root cause: _poll_job_status (cli.py:1380-1384) reuses the --diagnose wording for the unknown-job case, but this method is only reached from _run_via_rest_api right after job_id = body.get("jobId") came back from POST /submit-job (cli.py:1346, :1358). A typo in the id is not possible at this call site.
Server side: CoordinatorService.submitJob does runningJobInfoIMap.put(jobId, ...) (CoordinatorService.java:1454) before jobSubmitFuture.complete(null) (:1463), and the REST submit path join()s that future (BaseService.java:1396-1401); JobMaster.cleanJob() calls jobHistoryService.storeFinishedJobState(this) before scheduleRemoveJobStateMaps() (JobMaster.java:1041-1046). So a found: false right after submit genuinely means the job disappeared from both maps — e.g. a master restart without persisted state, or finished-job history expiring during the 5-minute poll — not a wrong id.
Impact: wording only, but it sends the user down the wrong troubleshooting path.
Suggested fix: word the message for the post-submit situation (job vanished from the cluster's running and finished maps; check master logs / /finished-jobs/:state) and keep the typo hint only in run_diagnose.
Evidence: seatunnel-cli/seatunnel_cli/cli.py:1382 f"The id may be wrong, or the job may have been evicted from the "; seatunnel-cli/seatunnel_cli/cli.py:1383 f"finished-job history."; seatunnel-cli/seatunnel_cli/cli.py:1677 f"[error]No job {escape(job_id)} on the cluster at "
Suggested fix: The id here came back from POST /submit-job a couple of seconds earlier (lines 1346/1358), so "The id may be wrong" cannot be the cause at this call site. Since CoordinatorService.submitJob puts the running JobInfo before completing the future the REST submit joins on, and JobMaster.cleanJob() stores the finished state before scheduling removal of the running maps, a found: false here really means the job disappeared from both maps (master restart without persisted state, or history expiry during the 5-minute poll). Could you word the message for that situation and keep the typo hint only in run_diagnose?
Severity: Low

Issue 6: UI fetch still has no error path: a rejected getJobInfo leaves the detail page blank and the new missing-job state never shows

Location: seatunnel-engine/seatunnel-engine-ui/src/views/jobs/detail.tsx:94
Why it matters: - Root cause: the new missing branch at detail.tsx:94-100 only handles a resolved 200 body. await getJobInfo(jobId) at line 93 has no try/catch, and fetch() is invoked at line 118 without a .catch. If the request rejects (a proxy 404, a 5xx during master failover, a network error), missing stays false, Object.assign(job, res) is never reached, no fetchTimer is scheduled, and the page renders the normal layout with an empty job object — a blank title/tag block, no retry, and an unhandled promise rejection in the console.
Impact: the PR's goal is to stop presenting "nothing" as a healthy page; the rejection path still does exactly that. Pre-existing, but the new branch is the natural place to close it.
Suggested fix: wrap the call in try/catch, render an error variant of the NResult (or a retry affordance), and add a mockRejectedValue spec next to the two new tests in detail.spec.ts.
Evidence: seatunnel-engine/seatunnel-engine-ui/src/views/jobs/detail.tsx:94 if (res.found === false || !res.jobStatus) {; seatunnel-engine/seatunnel-engine-ui/src/views/jobs/detail.tsx:95 missing.value = true
Suggested fix: This covers the resolved-200 case well, but await getJobInfo(jobId) on the line above is still uncaught. If the request rejects (proxy 404, 5xx while the master fails over, network error) missing stays false, no fetchTimer is scheduled, and the user gets a blank header with an unhandled rejection rather than either the not-found result or a retry. Since this PR is specifically about not showing 'nothing' as a healthy state, I'd wrap this in try/catch, render an error variant of the NResult, and add a mockRejectedValue spec next to the two new tests.
Severity: Low

Issue 7: JobsApi.job_exists returns False for any non-dict payload, conflating a non-JSON response with "job does not exist"

Location: tools/seatunnel-python-sdk/seatunnel/endpoints/jobs.py:100
Why it matters: - Root cause: Client.request (client.py:66-67) returns resp.text when the Content-Type header does not contain application/json. job_exists then hits if not isinstance(details, dict): return False (jobs.py:99-100) and silently reports the job as absent.
Impact: a proxy or gateway that rewrites the content type, or an auth/HTML page served with HTTP 200, makes job_exists() answer "no such job" for every id — the same silent false-negative this PR is removing from /job-info, just moved into the SDK. The caller has no way to tell it apart from a genuinely unknown job.
Suggested fix: raise SeaTunnelAPIError (or TypeError) for a non-dict body instead of returning False, and add a test in test_sdk.py with a text/plain body.
Evidence: tools/seatunnel-python-sdk/seatunnel/endpoints/jobs.py:100 return False; tools/seatunnel-python-sdk/seatunnel/endpoints/jobs.py:103 return "jobStatus" in details; tools/seatunnel-python-sdk/seatunnel/endpoints/jobs.py:98 details = self.get_job_details(jobId)
Suggested fix: Client.request returns resp.text when the Content-Type isn't application/json, so a proxy/gateway rewriting headers makes job_exists() return False for every id with no way for the caller to tell it apart from a genuinely unknown job. I'd raise SeaTunnelAPIError (or TypeError) here instead of returning False, and add a test with a text body.
Severity: Low

Issue 8: The UI "Job not found" state does not show which job id was looked up

Location: seatunnel-engine/seatunnel-engine-ui/src/views/jobs/detail.tsx:577
Why it matters: - Root cause: detail.tsx:571-581 renders NResult with t('detail.notFound.title') and t('detail.notFound.description') and returns before the header that normally prints job.jobId; the locale strings added in locales/en_US/detail.ts:22-26 and locales/zh_CN/detail.ts:22-25 have no {jobId} placeholder.
Impact: a user who lands on /jobs/<id> via a stale bookmark or a pasted link sees a generic 404 card with no id to copy into /finished-jobs/:state or seatunnel-cli --diagnose, which is exactly the next step the updated troubleshooting table in rest-api-job-lifecycle.md recommends. Minor UX.
Suggested fix: jobId (route param, detail.tsx:85) is already in scope; pass it through, e.g. t('detail.notFound.description', { jobId }), and add a {jobId} placeholder to both locale files.
Evidence: seatunnel-engine/seatunnel-engine-ui/src/views/jobs/detail.tsx:577 description={t('detail.notFound.description')}; seatunnel-engine/seatunnel-engine-ui/src/locales/en_US/detail.ts:25 'The job id is unknown to this cluster, or the job has expired from the finished-job history.'
Suggested fix: Minor UX point: this branch returns before the header that normally prints job.jobId, so the card never tells the user which id was unknown. jobId (route param, line 85) is in scope — could you pass it through, e.g. t('detail.notFound.description', { jobId }) with a {jobId} placeholder in both locale files? That gives the user the id to paste into /finished-jobs/:state or seatunnel-cli --diagnose, which is what the troubleshooting docs suggest next.
Severity: Low

Review conclusion

Conclusion: final approval remains pending on the previous comments

2. Remaining follow-ups

Issue 1: The new found: false response is only documented for /job-info/:jobId in rest-api-v2.md; the same payload is also served on the deprecated /running-job/:jobId (v2) and on both v1 endpoints, whose docs still show {"jobId": ""}
Issue 2: CLI post-submit polling now aborts on the first status-less / non-dict body, turning a single odd response into a permanent 'job not known' verdict
Issue 3: found: false is also emitted by the v1 /hazelcast/rest/maps/job-info/:jobId and deprecated /running-job/:jobId endpoints, but only the v2 /job-info docs were updated
Issue 4: CLI post-submit polling terminates on the first ambiguous body and reports a non-object payload as "job is not known to the cluster"
Issue 5: Post-submit polling message says "The id may be wrong" for an id the CLI itself just received from /submit-job
Issue 6: UI fetch still has no error path: a rejected getJobInfo leaves the detail page blank and the new missing-job state never shows
Issue 7: JobsApi.job_exists returns False for any non-dict payload, conflating a non-JSON response with "job does not exist"
Issue 8: The UI "Job not found" state does not show which job id was looked up

Thanks for the updates so far. Happy to discuss the remaining points.

DanielLeens

Thank you for picking this up, @wgzhao, and for following the compatibility direction agreed in #12785 so closely. I reviewed the full diff at head 54944fa7 together with the server call chain, the Web UI, the Python SDK and the CLI.

What Problem Does This PR Solve?

User pain point: GET /job-info/{jobId} returns HTTP 200 with only jobId for an id the cluster does not know, so a client that checks only the status code reports a missing job as healthy.
Fix approach: keep HTTP 200 and all successful payloads unchanged, add an explicit found: false marker to the unknown-id response, and teach the Web UI, Python SDK and CLI to use it (with a jobStatus-absence fallback for older servers).
One-sentence summary: an additive, backward-compatible found: false marker plus consistent client handling of unknown or expired job ids.

1. Code Change Review

1.1 Core Logic Analysis

Server (JobInfoService.java:114-116, RestConstant.java:25):

// before
return new JsonObject().add(RestConstant.JOB_ID, jobId.toString());
// after
return new JsonObject()
        .add(RestConstant.JOB_ID, jobId.toString())
        .add(RestConstant.FOUND, false);

Key findings:

The marker is added only in the final else branch, after the running map (IMAP_RUNNING_JOB_INFO) and the finished-state map (IMAP_FINISHED_JOB_STATE) both miss, so running and finished payloads cannot gain the field. This matches the accepted proposal exactly.
I checked the two windows that could make a live job look "unknown": (a) submit: CoordinatorService puts the JobInfo into runningJobInfoIMap before jobSubmitFuture.complete(null) (CoordinatorService.java around line 1454), so a job-info call right after submit-job returns already sees the job; (b) completion: JobMaster.cleanJob() (JobMaster.java around line 1042-1046) calls storeFinishedJobState before the delayed running-state cleanup removes the entry from the running map, so there is no gap between "running" and "finished". The normal path therefore never produces a false found: false.
Both the v2 servlet (JobInfoServlet.java:52) and the legacy v1 handler (RestHttpGetCommandProcessor.java:271) call the same getJobInfoJson, so both endpoints get the marker.

Runtime path:

GET /job-info/{id}
  -> JobInfoServlet / RestHttpGetCommandProcessor
  -> JobInfoService.getJobInfoJson(id)
       running map hit   -> full payload (no "found")
       finished map hit  -> full payload (no "found")
       both miss         -> {"jobId": id, "found": false}   (HTTP 200)
  -> UI detail.tsx:94 / SDK job_exists() / CLI _poll_job_status + run_diagnose
       found == false or no jobStatus -> "job not known" state, stop polling

Clients:

Web UI (detail.tsx:94-101): found === false || !jobStatus sets missing, clears both timers and the pin store, and returns before parse(res.createTime ...), which is the original crash path. The older-server fallback (!res.jobStatus) is covered.
Python SDK (jobs.py:90-103): job_exists() prefers found and falls back to "jobStatus" in details; existing get_job_details behavior and exceptions are untouched.
CLI: run_diagnose (cli.py:1676) checks the explicit marker first and keeps the status-less fallback; _poll_job_status (cli.py:1373-1385) now stops with a clear message instead of looping on UNKNOWN.

1.2 Compatibility Impact

Fully compatible. HTTP status and every successful payload are unchanged; the only wire change is one extra boolean field on a response that previously carried only jobId. Strict clients that deserialize the old unknown-id body will see one extra key, which is the same trade-off already accepted in #12785. New clients still work against older engines through the jobStatus fallback, which the UI, SDK and CLI tests exercise.

1.3 Performance / Side-Effect Analysis

No additional lookups or allocations beyond one boolean on the miss path. The UI now stops its 5 s polling timer for a missing job, which is a small reduction in request volume. The CLI no longer spins for up to 150 polls on an unknown id.

1.4 Error Handling and Logging

Issue 1

Location: seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/rest/service/JobInfoServiceNullSafetyTest.java:167-219
Problem: the maintainers asked for a running / finished / unknown / expired test matrix. Finished (with and without metrics) and unknown are asserted, but there is no test that a running id (the IMAP_RUNNING_JOB_INFO branch, JobInfoService.java:79-83) omits found. The "expired" case is the same code path as "unknown", so it is covered by shouldMarkUnknownJobAsNotFound, but it would help to say so in the test comment.
Potential risk: a future change that adds the marker in a shared helper could leak found into the running payload without any test turning red.
Best improvement: add one test that stubs runningJobInfoMap.get(jobId) with a JobInfo and asserts result.get(RestConstant.FOUND) is null.
Severity: Low
Raised by another reviewer: No

Issue 2

Location: docs/en/engines/zeta/rest-api-v1.md (the /hazelcast/rest/maps/job-info/:jobId section, around line 294) and the zh counterpart
Problem: the legacy v1 endpoint shares getJobInfoJson and therefore also returns found: false, but only the v2 and lifecycle docs describe it.
Potential risk: readers of the v1 page will not learn about the marker; low impact because v1 is already deprecated for this endpoint.
Best improvement: add one sentence to the v1 section (en and zh) pointing to the v2 description.
Severity: Low
Raised by another reviewer: No

Issue 3

Location: seatunnel-cli/seatunnel_cli/cli.py:1373-1385
Problem: polling now treats the first status-less response as terminal. I verified above that the normal submit and finish handoffs do not produce a status-less 200, so this is fine on the happy path; it would only matter during a master failover while the running-info map is being recovered.
Potential risk: a rare false "not known" message during failover, after which the user can simply re-query.
Best improvement: optional; tolerate one retry before declaring the job unknown, or leave as is and keep the current message (which already tells the user the id may be wrong or evicted).
Severity: Low
Raised by another reviewer: No

2. Code Quality Assessment

2.1 Coding Standards

Small, focused, follows existing constants and style. The new SDK method and server change are documented in docstrings and docs. RestConstant.FOUND is the right place for the key.

2.2 Test Coverage and Test Stability

Server: shouldMarkUnknownJobAsNotFound asserts found == false and no jobStatus; two existing finished-state tests now assert found is absent. UI: two new detail.spec.ts cases (explicit marker with polling stopped, and older-server status-less response). SDK: three job_exists cases (explicit marker, old running shape, old status-less shape). CLI: diagnose with the marker and parametrized polling for both response shapes. The UI test uses fake timers and restores real timers afterwards, and I see no Thread.sleep or wall-clock dependence. Stability: Stable. The only gap is the running-id assertion (Issue 1).

2.3 Documentation Updates

en and zh are both updated for REST v2, the job-lifecycle troubleshooting table and the Python SDK page; the new text matches the code. The release note is in the PR description, as requested in #12785; since the change is purely additive I agree it does not need an incompatible-changes.md entry. Only the v1 page is missing (Issue 2).

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix. The marker is produced at the single place where absence is determined, and clients branch on it.

3.2 Maintainability

Clear constant, clear fallback rule documented in three places. The UI missing-state is isolated behind one missing ref.

3.3 Extensibility

Leaves room for the staged 404 decision discussed in #12785 without committing to it, and keeps /jobs/checkpoints/{jobId} out of scope as agreed.

3.4 Historical-Version Compatibility

New clients against old engines and old clients against new engines both behave correctly (see 1.2). No config, option or serialization format changes.

4. Issue Summary

Number	Issue	Location	Severity
1	No test that a running id omits `found`	JobInfoServiceNullSafetyTest.java:167-219	Low
2	v1 REST docs do not mention the marker	docs/en/engines/zeta/rest-api-v1.md (job-info section)	Low
3	CLI polling stops on first status-less response (failover window only)	cli.py:1373-1385	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers - must be fixed
- CI is not green on this head: the Build check is timed_out / cancelled, and the only non-successful job in the fork run is mysql-cdc-connector-it (11), cancelled after its 2 hour budget. That is unrelated to this diff (no CDC code is touched). The branch is also 27 commits behind dev; please sync the latest dev and re-run the failed job only (a full re-run is not needed).
Recommended fixes - non-blocking
- Issues 1 to 3 above.

Overall assessment: the implementation matches the scope agreed in #12785 (additive found: false, 200 retained, checkpoint endpoint untouched), and I found no source-level blocker on the server, UI, SDK or CLI paths. Once CI is green on a freshly synced head this is ready for a maintainer's final look.

Better alternative: none needed; the staged 404 remains a separate future decision as agreed in the issue thread.

wgzhao · 2026-10-20T04:03:27Z

Addressed the review follow-ups:

Added a server test for the running-job branch; running and finished payloads are asserted to
omit found.
Documented the marker in the EN/ZH v1 REST pages, which share JobInfoService.
CLI polling now tolerates one status-less 200 (the failover window on older servers) before
declaring the job unknown; found: false still stops immediately.
Rebased onto the latest dev (1ce2d7815).

Validation on the new head: JobInfoServiceNullSafetyTest 8 tests, UI detail.spec.ts 3 tests
plus vue-tsc, Python SDK 10 tests, CLI test_diagnose_cli.py 29 tests; spotless:check and
the engine-server verify pass. The previous Build failure was the unrelated
mysql-cdc-connector-it (11) timeout; CI is re-running on the synced head.

wgzhao · 2026-10-20T05:04:19Z

Thanks for the detailed review. I addressed the remaining items:

Documented found: false in the deprecated v2 /running-job/:jobId blocks and in both v1
blocks (EN/ZH), which share JobInfoService.
CLI polling now retries one ambiguous response. A repeated status-less body gets a distinct
response-shape/failover message, and the post-submit path no longer claims the id may be wrong.
The Web UI now catches a rejected getJobInfo and renders an error result instead of a blank
page; the missing-job card also shows the requested job id.
JobsApi.job_exists raises TypeError for a non-JSON body instead of returning False.

Validation on the new head 3908e3bf1: server JobInfoServiceNullSafetyTest 8 tests, UI
detail.spec.ts 4 tests plus vue-tsc, Python SDK 11 tests, CLI test_diagnose_cli.py 29
tests; spotless:check and the engine-server verify pass.

DanielLeens

Thanks again for the quick turnaround, @wgzhao. I re-read the full diff at the new head 3908e3bf, and I also re-checked the server call chain, the Web UI, the Python SDK and the CLI. Good news first: all three Low items from my earlier review (running-id test, v1 docs, CLI failover tolerance) are addressed, and so are the follow-ups SEZ9 raised, and I found no source-level blocker.

What Problem Does This PR Solve?

User pain point: GET /job-info/{jobId} answers HTTP 200 with only jobId for an id the cluster does not know, so status-code-only checks report a missing job as healthy, the Web UI renders undefined timestamps, and --diagnose could say "Nothing to report".
Fix approach: keep HTTP 200 and all successful payloads unchanged, add an additive found: false marker on the unknown-id branch, and make the Web UI, Python SDK and CLI prefer the marker (with a "no jobStatus" fallback for older engines).
One-sentence summary: an additive, backward-compatible unknown-job marker with consistent client handling.

1. Code Change Review

1.1 Core Logic Analysis

Jetty v2 /job-info/:jobId  and  /running-job/:jobId  and  v1 /hazelcast/rest/maps/{job-info,running-job}
  -> JobInfoService.getJobInfoJson(Long)
       IMAP_RUNNING_JOB_INFO hit        -> running payload (no marker)
       IMAP_FINISHED_JOB_STATE hit      -> finished payload (no marker)
       both miss                        -> {jobId, found:false}   <- only changed branch

The server change is a single branch (JobInfoService.java:114-116, constant RestConstant.java:25). Running and finished payloads cannot gain the field, and the new test shouldNotAddFoundMarkerToRunningJobPayload now locks that in. Before/after:

// before
return new JsonObject().add(RestConstant.JOB_ID, jobId.toString());
// after
return new JsonObject().add(RestConstant.JOB_ID, jobId.toString()).add(RestConstant.FOUND, false);

Changes since my last review: running-branch test, v1 and deprecated v2 /running-job docs (en and zh), CLI polling now tolerates one status-less 200 and a repeated one gets a distinct message, the UI catches a rejected getJobInfo and shows an error result, and JobsApi.job_exists raises TypeError for a non-object body instead of returning False.

1.2 Compatibility Impact

Fully compatible. New clients on old engines fall back to "no jobStatus"; old clients on new engines ignore the extra key. No config option, default or serialization change.

1.3 Performance / Side-Effect Analysis

No hot-path impact. The only runtime side effect is the UI now stops polling (clears both timers and the pin store) when the job is reported missing, which matches the terminal-state branch.

1.4 Error Handling and Logging

The new UI try/catch stops polling on any rejection and shows the message, which is an improvement over the previous unhandled rejection. No sensitive data is logged.

Issue 1

Location: seatunnel-engine/seatunnel-engine-ui/src/views/jobs/detail.tsx:96 (fetch(): if (res.found === false || !res.jobStatus))
Problem: a single status-less 200 from an older engine permanently shows "Job not found" and stops polling, while the CLI now deliberately tolerates one such response for the master-failover window. The UI has no equivalent tolerance, and the user must reload the page.
Risk: only in the old-server failover window; a visible job can flip to "not found" until reload.
Best improvement: optional; for the status-less (not found:false) shape only, retry once on the next 5s tick before switching to the missing state.
Severity: Low
Raised by another reviewer: No

Issue 2

Location: seatunnel-engine/seatunnel-engine-ui/src/views/jobs/detail.tsx (render function, the part of the original JSX below the new return ()
Problem: the pre-existing JSX body was wrapped in a new return ( but not re-indented, so it no longer matches the repo Prettier style (npm run format would rewrite it). CI only runs the type-check build, so this will not fail, but it will create noise in the next UI change.
Best improvement: run prettier --write on the file.
Severity: Low
Raised by another reviewer: No

2. Code Quality Assessment

2.1 Coding Standards

Small and focused; the constant lives in RestConstant, and the new SDK method and CLI branches are commented with the reason for the fallback.

2.2 Test Coverage and Test Stability

Server: unknown id asserts found == false and no jobStatus; running and both finished shapes assert found is absent. UI: four detail.spec.ts cases (marker, status-less fallback, rejection, no extra polling after the marker). SDK: four job_exists cases. CLI: marker in diagnose, parametrized polling, and recovery after one status-less response. I see no Thread.sleep, fixed ports or wall-clock dependence (UI uses fake timers and restores real timers). Stability: Stable.

2.3 Documentation Updates

en and zh are updated for REST v1, v2 (including the deprecated /running-job blocks), the lifecycle troubleshooting table and the Python SDK page. The text matches the code. Purely additive, so no incompatible-changes.md entry is needed.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix at the single place where absence is determined.

3.2 Maintainability

One constant, one documented fallback rule.

3.3 Extensibility

Leaves the staged 404 decision from #12785 open.

3.4 Historical-Version Compatibility

Both old-client/new-server and new-client/old-server combinations behave correctly.

4. Issue Summary

Number	Issue	Location	Severity
1	UI treats one status-less 200 as permanent "not found" (CLI tolerates one)	detail.tsx fetch()	Low
2	Re-wrapped JSX body not re-indented (Prettier)	detail.tsx render	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers - must be fixed
- CI is not green on this head. In the fork run for 3908e3bf the unit-test jobs (Java 11/17, ubuntu and windows) fail in seatunnel-transforms-v2 with LLMBooleanOutputTest.readsLegacyFactoryWrapperWithValidationDisabled (InvalidClassException ... AbstractMultiCatalogTransform; local class incompatible, serialVersionUID mismatch), and all-connectors-it-6 fails in FakeSourceToAssertIT.testFakeSourceToAssertSink (a ParameterResolution error). Neither touches code in this PR. The first looks like a base issue: the branch is already rebased on dev 1ce2d7815 (0 commits behind), which contains #12194 where that test was introduced, so I expect it to fail on dev too, though I could not confirm a green/red dev run. I did not find an open fix PR for it. Please re-run only the failed jobs once dev is fixed; the second looks like an IT flake.
Recommended non-blocking fixes
- Issues 1 and 2 above.

Overall assessment: the implementation matches the agreed scope and I found no source blocker. This is ready for a maintainer's final look once CI is green on a head that includes a fix for the dev unit-test failure.

Better alternative: none needed.

wgzhao · 2026-10-20T11:37:02Z

Thanks. Both remaining items are addressed in the latest head 1747a27d7:

The Web UI now tolerates one status-less response from an older engine (the failover window)
and retries on the next 5 s tick; a second consecutive status-less response switches to the
missing state immediately. found: false still switches immediately. Covered by a new
detail.spec.ts recovery test (5 UI tests total).
The render structure was changed to keep the original JSX body at its existing indentation.
prettier --check now passes for the changed UI files.

CI note: the LLMBooleanOutputTest failure is a base issue. This PR has no
seatunnel-transforms-v2 changes, and the same
InvalidClassException ... AbstractMultiCatalogTransform (serialVersionUID mismatch) reproduces
locally on the rebased head. The FakeSourceToAssertIT failure is an unrelated E2E flake. dev
is still at 1ce2d7815, so there is no upstream fix to rebase onto yet.

DanielLeens

Thank you for the quick follow-up, @wgzhao. I re-read the full diff at the new head 1747a27d and re-checked the server call chain, the Web UI, the Python SDK and the CLI. Both Low items from my previous review (the UI having no tolerance for one status-less 200, and the un-indented JSX body) are addressed. The earlier points from the first round (running-id test, v1 docs, CLI failover tolerance) and SEZ9's documentation follow-ups also stay resolved. I found no source-level blocker.

What Problem Does This PR Solve?

User pain point: GET /job-info/{jobId} answers HTTP 200 with only jobId for an id the cluster does not know, so status-code-only checks report a missing job as healthy, the Web UI renders undefined timestamps, and --diagnose could say "Nothing to report".
Fix approach: keep HTTP 200 and every successful payload unchanged, add an additive found: false marker on the unknown-id branch, and make the Web UI, Python SDK and CLI prefer the marker (with a "no jobStatus" fallback for older engines).
One-sentence summary: an additive, backward-compatible unknown-job marker with consistent client handling.

1. Code Change Review

1.1 Core Logic Analysis

Runtime path (engine REST, verified at the PR head):

Jetty v2 /job-info/:jobId and /running-job/:jobId, v1 /hazelcast/rest/maps/{job-info,running-job}
  -> JobInfoService.getJobInfoJson(Long)            (JobInfoService.java:77)
       IMAP_RUNNING_JOB_INFO hit    -> convertToJson(...)         running payload, no marker
       IMAP_FINISHED_JOB_STATE hit  -> finished payload (metrics default to JobMetrics.empty() when missing), no marker
       both miss                    -> {jobId, found:false}       (JobInfoService.java:114-116, the only changed branch)
  -> HTTP 200
Clients: detail.tsx fetch() / JobsApi.job_exists() / CLI _poll_job_status() + run_diagnose()

Server: one branch, one constant (RestConstant.FOUND). Running and finished payloads cannot gain the field. Note that a retained finished job whose metrics entry is missing still goes through the finished branch with empty metrics, so it carries jobStatus and is correctly not reported as missing by any client.
Web UI (detail.tsx): found === false switches to the missing state immediately and clears both timers plus the pin store. A status-less body (older engine) is tolerated once via statusLessSeen (retry on the next 5 s tick) and a second consecutive one switches to missing; the flag is reset after a good response. The rejected getJobInfo path is now caught. This resolves my earlier Issue 1.
Python SDK: job_exists() prefers found, falls back to "jobStatus" in details, and raises TypeError for a non-object body instead of silently reporting "does not exist".
CLI: _poll_job_status stops at once on found: false, tolerates one status-less body, and prints a distinct response-shape message on a repeat; run_diagnose checks the marker first and keeps the old status-less fallback.

1.2 Compatibility Impact

Fully compatible. HTTP status and all successful payloads are unchanged; the only wire change is one extra boolean on the unknown-id body. New clients against older engines fall back to the jobStatus check; old clients against new engines ignore the extra key. No config option, default or serialization change, and the staged 404 decision from #12785 stays open.

1.3 Performance / Side-Effect Analysis

No hot-path impact (one boolean on the miss path). The UI stops its 5 s poll for a missing job and the CLI no longer spins up to 150 polls on an unknown id.

1.4 Error Handling and Logging

Issue 1

Location: seatunnel-engine/seatunnel-engine-ui/src/views/jobs/detail.tsx:808-813 (the new catch in fetch()) and :851 (return () => (missing.value || loadError.value ? renderMissing() : renderContent()))
Problem: loadError replaces the whole detail page. If a job has already been loaded and is being polled every 5 s, one transient network error (or a restart blip on the master) swaps a fully populated page for an error card and stops polling. Before this PR the same rejection was unhandled, so the page stayed on the last good data (polling also stopped, but the user kept seeing the job).
Risk: Low. Only affects a running job viewed during a transient REST failure; the user can reload.
Best improvement: show the error card only while nothing has been loaded yet (for example when job.jobStatus is still empty); once data exists, keep it and either retry on the next tick or show a lightweight message toast.
Severity: Low
Raised by another reviewer: No

Issue 2

Location: seatunnel-engine/seatunnel-engine-ui/src/tests/detail.spec.ts (new cases)
Problem: a small test-scope note only. The new UI cases cover marker, status-less fallback, rejection, no extra polling and recovery; there is no case for "a rejection after a successful first load", which is the behavior described in Issue 1.
Best improvement: add one assertion once Issue 1 is decided, so the intended behavior is pinned.
Severity: Low
Raised by another reviewer: No

2. Code Quality

2.1 Coding Standards

Small and focused. The constant lives in RestConstant; the fallback and failover-tolerance branches carry short comments explaining why. No new core Java method was added, so there is no missing-comment issue.

2.2 Test Coverage and Test Stability

Server (JobInfoServiceNullSafetyTest.java): shouldMarkUnknownJobAsNotFound asserts found == false and no jobStatus; both finished shapes and shouldNotAddFoundMarkerToRunningJobPayload assert found is absent. These can genuinely fail if the marker leaks into the running or finished branch or is dropped on the miss branch.
UI (detail.spec.ts, 5 cases), SDK (test_sdk.py) and CLI (test_diagnose_cli.py) tests exercise real branches, not only mocks. No Thread.sleep, fixed ports or wall-clock dependence (UI uses fake timers).
Stability rating: Stable.

2.3 Documentation

en and zh are updated for REST v1, REST v2 (including the deprecated /running-job), the job-lifecycle page and the Python SDK page. The wording is consistent with the code.

3. Architectural Soundness

The fix sits at the single place where "absent" is decided and keeps the contract additive. Consumers share one rule (marker first, jobStatus absence as the legacy fallback). No better alternative is needed at this scope.

4. Issue Summary

Number	Issue	Location	Severity
1	A transient rejection after a successful load replaces the populated detail page with an error card	detail.tsx:808-813, :851	Low
2	No UI test pins the post-load rejection behavior	detail.spec.ts	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers - must be fixed
- CI is not green on this head, and I could not find a source-level cause in this PR. In the fork run for 1747a27d (https://github.com/wgzhao/seatunnel/actions/runs/39416195146) the four unit-test jobs fail in seatunnel-transforms-v2 at LLMBooleanOutputTest.readsLegacyFactoryWrapperWithValidationDisabled (InvalidClassException ... AbstractMultiCatalogTransform, introduced by #12194 on dev); all-connectors-it-7 failed on a Maven Connection reset while resolving commons-math3 for seatunnel-translation-spark-2.4; engine-v2-it failed in BackpressureSlowSinkIT.testCheckpointsKeepCompletingUnderSustainedBackpressure; two further jobs were cancelled. None of these touch files in this PR. Please re-run only the failed jobs once the LLMBooleanOutputTest problem is fixed on dev (I did not find a merged fix; the branch is 0 commits behind dev).
Recommended non-blocking fixes
- Issues 1 and 2 above.

Overall assessment: the implementation matches the agreed scope and I found no non-CI blocker. It is ready for a maintainer's final look once CI is green on a head that includes a dev fix.

Better alternative: none needed.

wgzhao · 2026-10-21T00:38:04Z

Both remaining Low items are addressed in the latest head 489dd37c5:

The full error card is now only used before the first successful load. Once a job is loaded, a
transient refresh rejection keeps the populated page, shows one warning for that error streak,
and retries on the next 5 s tick. The last good data stays visible.
Added a UI test for a rejection after a successful load; detail.spec.ts now has 6 cases.

vue-tsc and prettier --check pass on the changed UI files.

CI note: dev is still at 1ce2d7815; the LLMBooleanOutputTest serialVersionUID failure and
the unrelated IT flakes remain base/environment issues. This PR has no
seatunnel-transforms-v2 changes.

wgzhao · 2026-10-21T02:26:27Z

CI watch update for head 489dd37c5:

engine-v2-it Java 11 and Java 17: success
Build SeaTunnel UI: success
The only failures in the Build are the four unit-test jobs, all in
LLMBooleanOutputTest with the AbstractMultiCatalogTransform serialVersionUID mismatch.
This PR has no seatunnel-transforms-v2 changes, and the same failure reproduces locally on
the rebased head.

The overall Build will stay red until the base dev unit-test issue is fixed; there are no
failures related to this PR's server, UI, SDK or CLI changes.

SEZ9 · 2026-10-21T02:56:39Z

Thanks @wgzhao, the 489dd37c5 approach for the UI error path (PR12904-F6) sounds right: full error card only before the first successful load, a populated page that survives a transient refresh rejection with a single warning per error streak, and retry on the next 5 s tick. Covering the rejection-after-successful-load case in detail.spec.ts is exactly the test I was hoping for.

Two small follow-ups on that scope:

Does the warning clear once a subsequent refresh succeeds again, so a recovered job does not keep showing a stale error banner? If yes, a one-line note (or an assertion in the new spec case) would make that explicit.
You mention "both remaining Low items" — I read that as F6 plus the "Job not found" state now showing the looked-up job id (PR12904-F8). Could you confirm F8 is in 489dd37c5 as well? If it was already handled in an earlier push, just point me at it.

For the rest of the earlier scope, I don't see status in this thread for the docs items (the found: false payload on the v1 /hazelcast/rest/maps/job-info/:jobId and deprecated /running-job/:jobId endpoints, PR12904-F1/F3), the CLI polling behaviour and the "The id may be wrong" message (PR12904-F2/F4/F5), or JobsApi.job_exists treating a non-dict payload as "not found" (PR12904-F7). If those were addressed in a previous head, a short summary of what changed where is enough; otherwise let me know which ones you intend to defer.

On CI: agreed that engine-v2-it on Java 11/17 and Build SeaTunnel UI passing for 489dd37c5 is what matters for this PR, and the LLMBooleanOutputTest serialVersionUID failures in the unit-test jobs are out of scope here given there are no seatunnel-transforms-v2 changes. Once dev moves past 1ce2d7815 with the fix, a rebase should turn the Build green; no action needed from you on that in this PR.

wgzhao · 2026-10-21T02:59:03Z

Thanks for the follow-up. Answers and locations:

Warning clearing: on every successful refresh loadError and lastRefreshError are reset,
so the warning is a transient toast and a recovered job does not keep a stale error. The
post-load rejection spec now includes a third, successful response and asserts that the job
updates and the refresh warning is gone afterwards.
F8: yes, the missing-state description is rendered with
t('detail.notFound.description', { jobId }), and the spec asserts that the route job id
(123456789) appears.
F1/F3 (docs): found: false is documented in EN/ZH v1 /job-info and /running-job, and
in the v2 /running-job blocks; each points at the full v2 description and the old-server
fallback.
F2/F4/F5 (CLI): _poll_job_status tolerates one status-less response, uses a distinct
response-shape/failover message on a repeat, and the post-submit path no longer says the id may
be wrong. run_diagnose checks found: false first and keeps the status-less fallback.
F7 (SDK): JobsApi.job_exists raises TypeError for a non-dictionary/non-JSON body instead
of reporting the job as absent.

Latest head: 86e750827. UI detail.spec.ts has 6 cases, including the post-load rejection and
recovery assertions; vue-tsc and prettier --check pass. The earlier engine-v2-it Java 11/17
and UI build results remain the relevant green checks; the only Build failures are the base
LLMBooleanOutputTest serialVersionUID issue.

DanielLeens

Thank you @wgzhao for the follow-up and for the careful answers to SEZ9's questions. I re-read the full diff at the new head 86e7508277b8805a0118d6e0b6804a7c5c06d70d. This is a single squashed commit (the branch was force-pushed and rebased since my last review at 1747a27d, so I reviewed the whole PR rather than an incremental diff). Both Low items from my last round are addressed in code: a transient refresh failure after a successful load now keeps the populated page, shows one warning per error streak and retries on the next tick, and the not-found card shows the requested job id. I did not find any source-level blocker.

What Problem Does This PR Solve?

Pain point: GET /job-info/{jobId} answers HTTP 200 with only jobId for an id the cluster does not know, so status-code-only checks treat a missing job as healthy, the Web UI parses undefined timestamps, and --diagnose could print "Nothing to report" (refs #12785).
Fix approach: Keep HTTP 200 and every successful payload unchanged; add an additive found: false marker on the unknown-id branch; make the Web UI, Python SDK and CLI prefer the marker with a "no jobStatus" fallback for older engines.
One sentence: An additive, backward-compatible unknown-job marker with consistent client handling.

1. Code Change Review

1.1 Core logic and runtime path

Jetty v2 /job-info/:jobId and deprecated /running-job/:jobId, v1 /hazelcast/rest/maps/{job-info,running-job}
  -> JobInfoService.getJobInfoJson(Long)
       IMAP_RUNNING_JOB_INFO hit     -> convertToJson(...)                       (unchanged, no marker)
       IMAP_FINISHED_JOB_STATE hit   -> finished payload (metrics default to JobMetrics.empty())  (unchanged)
       both miss                     -> {jobId, found:false}                      (the only changed branch, JobInfoService.java:114-116)
  -> HTTP 200
Clients: detail.tsx fetch() | JobsApi.job_exists() | CLI _poll_job_status() and run_diagnose()

Server: one branch and one constant (RestConstant.FOUND). The running and finished branches cannot gain the field, which shouldNotAddFoundMarkerToRunningJobPayload and the finished-job assertions pin.
Web UI (detail.tsx:95-155): found === false switches to the missing state and clears both timers and the pin store; a status-less body (older engine) is tolerated once via statusLessSeen and a repeat switches to missing; a rejection before the first load shows the error card, a rejection after a successful load keeps the data, warns once per distinct error and reschedules; statusLessSeen/lastRefreshError are reset on a good response. useMessage() is safe because App.tsx wraps the app in NMessageProvider.
Python SDK: job_exists() prefers found, falls back to "jobStatus" in details, and raises TypeError for a non-object body rather than reporting the job as absent.
CLI: _poll_job_status stops at once on found: false, tolerates one status-less body (the loop continue still goes through the top-of-loop sleep), and prints a distinct response-shape/failover message on a repeat; run_diagnose checks the marker first and keeps the legacy fallback.

1.2 Compatibility

Fully compatible. HTTP status and all successful payloads are unchanged; the only wire change is one extra boolean on the unknown-id body. New clients on older engines fall back to the jobStatus check; old clients ignore the extra key. No option, default or serialization change, and the 404 question from #12785 stays open as agreed. The Hazelcast v1 handler routes to the same JobInfoService, so v1 gets the marker too, and the v1 docs now say so.

1.3 Performance and side effects

Nothing on a hot path (one boolean on the miss branch). The UI no longer polls a missing job every 5 s, and the CLI no longer spins up to 150 polls on an unknown id.

1.4 Error handling, logging, and issues

Issue 1 (Medium, not a blocker): the new "refresh warning clears" assertion cannot fail, and the warning itself is never asserted

Location: seatunnel-engine/seatunnel-engine-ui/src/tests/detail.spec.ts, test should keep the loaded job when a refresh is rejected (the last expect(document.body.textContent).not.toContain('Could not refresh job')).
Problem: The test advances fake timers by 5 s twice. The warning is a message.warning(...) toast, which naive-ui removes on its own after its default duration, so after the second advanceTimersByTime(5000) the text is gone whether or not the production code ever clears lastRefreshError/loadError. In addition, there is no positive assertion that the warning was shown after the rejected refresh, and nothing checks the "one warning per error streak" rule (a second identical rejection must not warn again; a different error must). If message.warning(...) were deleted from the catch block, or the lastRefreshError !== error guard were removed, this test would still pass.
Risk: The stated behavior (single warning per streak, reset on recovery) is not actually protected by a test that can go red.
Best improvement: After the rejected refresh, assert the warning text is present (expect(document.body.textContent).toContain('Could not refresh job')) before advancing again; add a second consecutive identical rejection and spy on useMessage().warning (or count the rendered toasts) to assert exactly one call; then assert that a changed error message warns again. Prefer asserting on the mocked message.warning call count over scraping the DOM, since toasts auto-dismiss.
Severity: Medium (test strength). Raised by another reviewer: No (SEZ9 asked whether the warning clears, and the answer given relies on this assertion).

Issue 2 (Low): small test-name drift

Location: JobInfoServiceNullSafetyTest.java, shouldReturnJobIdOnlyWhenFinishedMetricsIsMissing.
Problem: Since the finished branch defaults missing metrics to JobMetrics.empty(), the result carries the full finished payload (not "jobId only"). The name is older than this PR, but the PR now adds an assertion to it, so it is a good moment to rename it (for example shouldReturnFinishedPayloadWithEmptyMetricsWhenMetricsAreMissing) so readers do not take it to be the unknown-id case.
Severity: Low. Raised by another reviewer: No.

2. Code Quality

2.1 Standards

Small and focused. The constant is in RestConstant, comments explain the old-server fallback and failover tolerance, and i18n keys exist in both en_US and zh_CN. No new core Java method was added, so there is no missing-comment issue. Nothing else to add.

2.2 Tests and stability

Test	Can it fail on a real bug?	Rating
`JobInfoServiceNullSafetyTest.shouldMarkUnknownJobAsNotFound` (`:207`)	Yes: fails if the miss branch drops `found:false` or gains `jobStatus`	Stable
`shouldNotAddFoundMarkerToRunningJobPayload` (`:221`) / finished-branch `assertNull(found)`	Yes: fails if the marker leaks into running or finished payloads. The anonymous subclass stub of `convertToJson` is explained in a comment	Stable
`detail.spec.ts` missing-state, two status-less, one status-less recovery, initial rejection	Yes: each exercises a distinct branch of `fetch()` with fake timers and call counts	Stable
`detail.spec.ts` refresh-rejected case	Only the "job stays visible and polling resumes" part can fail; the warning part cannot (Issue 1)	Risk present (narrow)
`test_diagnose_cli.py` (marker, polling stop, status-less recovery)	Yes: asserts call counts (1 vs 2) and output text; sleep is monkeypatched	Stable
`test_sdk.py` `job_exists` (marker, fallback, status-less, non-JSON)	Yes: four distinct decisions, including the `TypeError` path	Stable

No Thread.sleep, fixed ports or wall-clock dependence. Overall: Stable, with the one narrow exception above.

2.3 Docs

docs/en and docs/zh are updated in lockstep for REST v1 /job-info and /running-job, REST v2 /job-info and the deprecated /running-job, the job-lifecycle troubleshooting row, and the Python SDK page. Wording matches the code (unknown id, HTTP 200, found: false, older-server fallback).

3. Architectural soundness

The marker is added at the one place where "absent" is decided, the contract stays additive, and all three clients share the same rule (marker first, missing jobStatus as the legacy fallback). No better alternative is needed at this scope.

4. Issue Summary

#	Severity	Issue	Blocker?
1	Medium	Refresh-warning assertion in `detail.spec.ts` cannot fail; warning/dedupe never asserted	No (recommended)
2	Low	Existing test name no longer describes its scenario	No

CI at this head: the apache-side Build check is a pointer to the author's fork run (wgzhao/seatunnel run 39536011910 for 86e75082), which was still queued when I looked, so there is no conclusion for this head yet; labeler and Notify test workflow passed. For the previous head, the author reported engine-v2-it (Java 11/17) and Build SeaTunnel UI green, with the only failures being the four unit-test jobs at LLMBooleanOutputTest (AbstractMultiCatalogTransform serialVersionUID, from seatunnel-transforms-v2, which this PR does not touch). That matches what I saw last round, and I did not find a merged fix on dev (dev is currently bab2d089c; this branch is 1 commit behind it with no file overlap). Please re-run only the failed jobs on this head once the base unit-test issue is resolved. mergeable_state is blocked (required checks pending).

5. Merge Recommendation

Ready to merge after fixes. There is no non-CI blocker; the implementation matches the agreed scope and is backward compatible. Remaining before merge:

Blockers:

The fork Build run must complete green for this head (the base LLMBooleanOutputTest failure is outside this PR; a rerun after a dev fix is needed).

Recommended fixes: Issue 1 (make the refresh-warning assertions able to fail) and Issue 2 (test rename).

Thanks again, this has been a very responsive iteration.

wgzhao · 2026-10-21T07:06:52Z

Both recommendations are addressed in the latest head 21780a760:

The refresh-warning test now mocks useMessage().warning and asserts the behavior rather than
relying on an auto-dismissed toast: the first error warns, an identical consecutive error does
not warn again, a changed error warns again, and after a successful refresh the same error warns
once more (proving the streak state was reset).
Renamed shouldReturnJobIdOnlyWhenFinishedMetricsIsMissing to
shouldReturnFinishedPayloadWithEmptyMetricsWhenMetricsAreMissing.

The branch is rebased onto the latest dev (bab2d089c). Validation: UI detail.spec.ts 7
tests, server JobInfoServiceNullSafetyTest 8 tests, vue-tsc, prettier --check and
spotless:apply pass.

The new head will re-run CI. bab2d089c is a Parquet/Spark fix, so it does not address the base
LLMBooleanOutputTest serialVersionUID failure.

DanielLeens

Thank you @wgzhao for the quick turnaround. I re-read the PR at the new head 21780a76015c, which is one squashed commit rebased onto dev at bab2d089c. Both recommendations from my last round are addressed, and I did not find any source-level blocker.

What Problem Does This PR Solve?

Pain point: GET /job-info/{jobId} answers HTTP 200 with only jobId for an id the cluster does not know. Status-code-only checks then treat a missing job as healthy, the Web UI parses undefined timestamps, and --diagnose can print "Nothing to report" (refs #12785).
Fix approach: Keep HTTP 200 and every successful payload unchanged. Add an additive found: false marker on the unknown-id branch, and make the Web UI, Python SDK and CLI prefer the marker with a "no jobStatus" fallback for older engines.
One sentence: An additive, backward-compatible unknown-job marker with consistent client handling.

1. Code Change Review

1.1 Core Logic Analysis

Incremental check of the two items I raised last time:

Refresh-warning test (was Medium): detail.spec.ts now hoists a warningSpy and mocks useMessage().warning (lines 28-41). The new test should warn once per error streak and reset after a successful refresh (lines 344-404) drives the sequence ok, blip, blip, other blip, ok, blip and asserts the call counts 0, 1, 1, 2, 2, 3. This pins every production branch in detail.tsx:139-154: the first error warns, an identical consecutive error is suppressed by the lastRefreshError !== error guard (line 149), a changed error warns again, and the reset at detail.tsx:122 is proven because the same blip warns again after the successful refresh. Deleting message.warning(...), removing the guard, or removing the reset would each turn this test red. The old not.toContain('Could not refresh job') DOM scrape is gone, and the earlier test now only asserts what it can actually fail on (job stays visible, polling resumes).
Test rename (was Low): shouldReturnFinishedPayloadWithEmptyMetricsWhenMetricsAreMissing now exists at JobInfoServiceNullSafetyTest.java:168.

Unchanged and re-verified on this head:

Jetty v2 /job-info/:jobId and deprecated /running-job/:jobId, v1 /hazelcast/rest/maps/{job-info,running-job}
  -> JobInfoService.getJobInfoJson(Long)
       running hit   -> convertToJson(...)         (unchanged, no marker)
       finished hit  -> finished payload           (unchanged)
       both miss     -> {jobId, found:false}       (only changed branch, JobInfoService.java:113-116)
  -> HTTP 200
Clients: detail.tsx fetch() (:95-155) | JobsApi.job_exists() | CLI _poll_job_status() / run_diagnose()

detail.tsx handles found === false (:98), one tolerated status-less body then missing (:106-120), and rejection before vs after the first load (:139-154). useMessage() is safe because App.tsx wraps the app in NMessageProvider.

1.2 Compatibility Impact

Fully compatible. HTTP status and all successful payloads are unchanged; the only wire change is one extra boolean on the unknown-id body. New clients fall back to the jobStatus check against older engines, and old clients ignore the extra key. No option, default or serialization change, and the 404 question from #12785 stays open as agreed.

1.3 Performance/Side-Effect

Nothing on a hot path (one boolean on the miss branch). The UI stops polling a missing job, and the CLI stops polling an unknown id.

1.4 Error Handling and Logging

No blocking issues. One optional nit:

Issue 1 (Low, optional): fake timers are restored only on the success path

Location: seatunnel-engine/seatunnel-engine-ui/src/tests/detail.spec.ts:158-404, the tests that call vi.useFakeTimers() and end with vi.useRealTimers().
Problem: If an assertion in one of these tests fails, useRealTimers() is skipped and later tests inherit fake timers, so one real failure can show up as several confusing ones.
Best improvement: Add afterEach(() => { vi.useRealTimers() }) in the describe block and drop the per-test calls. This is test hygiene only and not a blocker.
Severity: Low

2. Code Quality Assessment

2.1 Coding Standards

Small and focused. The constant lives in RestConstant, comments explain the older-server fallback and the failover tolerance, and i18n keys exist in both en_US and zh_CN (including the new refresh message). No new core Java method was added.

2.2 Test Coverage and Test Stability

Test	Can it fail on a real bug?	Rating
`JobInfoServiceNullSafetyTest` unknown/running/finished marker tests	Yes: fail if the marker is dropped from the miss branch or leaks into running or finished payloads	Stable
`detail.spec.ts` missing, two status-less, one status-less recovery, initial rejection	Yes: each hits a distinct branch with fake timers and call counts	Stable
`detail.spec.ts` refresh rejected / warn once per streak	Yes: spy call counts 0,1,1,2,2,3 pin the guard and the reset	Stable
`test_diagnose_cli.py`, `test_sdk.py`	Yes: assert call counts, output text and the `TypeError` path	Stable

Overall rating: Stable. No Thread.sleep, fixed ports or wall-clock dependence; timers are faked, and the one hygiene nit is Issue 1.

2.3 Documentation Updates

docs/en and docs/zh are updated in lockstep for REST v1 and v2 (/job-info and the deprecated /running-job), the job-lifecycle row and the Python SDK page.

3. Architectural Soundness

3.1 Precise fix / Temporary workaround / Long-term solution

Precise fix at the one place where "absent" is decided, with an additive contract.

3.2 Scope

Focused on one problem, with all three clients following the same rule (marker first, missing jobStatus as the legacy fallback).

3.3 Alternatives

Returning 404 would be a behavior change for existing callers; it remains an open, separate discussion in #12785.

3.4 Historical-version compatibility

Old clients on a new engine and new clients on an old engine both keep working, as described in 1.2.

4. Issue Summary

Number	Issue	Location	Severity
1	Fake timers restored only on the success path (optional hygiene)	`detail.spec.ts:158-404`	Low

CI at this head: the apache-side Build check is still in progress, and labeler and Notify test workflow passed. For the previous head the failures were the four unit-test jobs at LLMBooleanOutputTest (AbstractMultiCatalogTransform serialVersionUID, in seatunnel-transforms-v2, which this PR does not touch). I could not confirm a fix for that on dev, so please wait for this run and re-run only the failed jobs if the same base failure appears. mergeStateStatus is BLOCKED (review required and required checks pending). The branch is one commit behind dev (a CodeQL workflow bump) with no file overlap, so no sync is needed for correctness.

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers:

The Build run for this head must finish green. The only known red items are the unrelated base LLMBooleanOutputTest failure, so a targeted rerun may be needed.

Recommended fixes: Issue 1 (optional).

Overall: both earlier recommendations are fixed with tests that can genuinely fail, and there is no non-CI blocker. Thanks again, this has been a very responsive iteration.

wgzhao · 2026-10-21T13:39:29Z

Thanks — the fake-timer hygiene nit is addressed in 3578e25a4: vi.useRealTimers() now runs in afterEach for every test in detail.spec.ts instead of only at the end of each success path. Verified locally with vitest (7/7 detail tests, 67/67 full UI suite), prettier --check and vue-tsc; CI is running on the new head. Would you be able to give a formal approval once the Build is green?

DanielLeens

Thank you @wgzhao for the quick fix of the fake-timer hygiene nit, and for the very responsive iteration on this PR. I re-read the whole PR from scratch at the new head 3578e25a4ff5 (two commits on top of dev at bab2d089c), not only the last delta. All eight earlier findings (the first round) and both recommendations from my last round are resolved, and I did not find a source-level blocker. The only remaining gate is CI, and the red items I looked at are not caused by this diff (details in section 4).

What Problem Does This PR Solve?

Pain point: GET /job-info/{jobId} answers HTTP 200 with only jobId when the id is neither running nor in finished-job history. Status-code-only checks then treat a missing job as healthy, the Web UI parses undefined timestamps and keeps polling, and --diagnose can print "Nothing to report" (refs #12785).
Fix approach: Keep HTTP 200 and every successful payload unchanged. Add an additive found: false marker on the unknown-id branch of JobInfoService, and teach the Web UI, Python SDK and CLI to prefer the marker with a "no jobStatus" fallback for older engines.
One sentence: An additive, backward-compatible unknown-job marker with consistent client handling.

1. Code Change Review

1.1 Core Logic Analysis

Server change (the only wire change), JobInfoService.java:111-117:

// before
return new JsonObject().add(RestConstant.JOB_ID, jobId.toString());
// after
return new JsonObject()
        .add(RestConstant.JOB_ID, jobId.toString())
        .add(RestConstant.FOUND, false);

Runtime path (re-verified on this head):

Jetty v2 /job-info/:jobId and deprecated /running-job/:jobId
Hazelcast v1 /hazelcast/rest/maps/{job-info,running-job}/{id}
  -> JobInfoService.getJobInfoJson(Long)
       running map hit   -> convertToJson(...)   (unchanged, no marker)
       finished map hit  -> finished payload     (unchanged, no marker)
       both miss         -> {jobId, found:false} (only changed branch)
  -> HTTP 200
Clients:
  Web UI   detail.tsx fetch()          found===false -> missing; no jobStatus -> tolerate one, then missing
  Python   JobsApi.job_exists()        "found" in details -> bool(found); else "jobStatus" in details; non-dict -> TypeError
  CLI      _poll_job_status()          found is False -> stop; status-less -> tolerate one, then stop with a distinct message
  CLI      run_diagnose()              found is False -> "marked it as unknown", exit 1; status-less fallback kept

Does the normal user path reach the changed logic? Yes for the Web UI, SDK and CLI consumers whenever a user asks about an id that is unknown or expired. Successful lookups (running or finished) never take the changed branch, so they cannot gain the field. The reported problem is real: the old miss branch really returned a bare {jobId} with HTTP 200.

Delta since my last review (21780a76015c -> 3578e25a4ff5): one test-only commit. detail.spec.ts now restores real timers in a single afterEach(() => { vi.useRealTimers() }) and the per-test vi.useRealTimers() calls are removed. This fully resolves my previous optional Issue 1: a failing assertion can no longer leak fake timers into later tests.

Status of earlier findings:

Earlier finding	Status
Docs for v1 `/job-info` and `/running-job` and v2 deprecated `/running-job` still showed `{"jobId": ""}`	Resolved (EN and ZH, each pointing to the v2 description)
CLI polling gave up on the first status-less or non-dict body	Resolved (one status-less 200 is tolerated, a repeat gives a distinct response-shape / failover message)
"The id may be wrong" message for an id the CLI itself received	Resolved (message removed)
UI `fetch` had no rejection path	Resolved (error card before first load, single warning per error streak after)
`job_exists` reported a non-JSON body as "does not exist"	Resolved (`TypeError`, covered by `test_job_exists_rejects_non_json_response`)
Not-found card did not show the job id	Resolved (`t('detail.notFound.description', { jobId })`)
Refresh-warning test could not fail (Medium, last round)	Resolved (`useMessage().warning` spy, counts 0,1,1,2,2,3)
Test name drift (Low, last round)	Resolved
Fake timers restored only on success path (Low, optional, last round)	Resolved in `3578e25a4`

1.2 Compatibility Impact

Fully compatible. HTTP status and all successful payloads are unchanged; the only wire change is one extra boolean on the unknown-id body. New clients fall back to the jobStatus check against older engines, and old clients ignore the extra key. No option, default, serialization or checkpoint-state change, and the 404 question stays open in #12785 as agreed.

1.3 Performance / Side-Effect Analysis

Nothing on a hot path: one boolean on the miss branch. The UI stops polling a missing job and the CLI stops polling an unknown id (previously up to 150 polls). No new lookups, threads or resources on the engine.

1.4 Error Handling and Logging

No blocking issues. Two optional Low observations that I should have raised in an earlier round (carryover; sorry for missing these in the last review):

Issue 1 (Low, optional): the duration ticker is stopped when a refresh fails after a successful load

Location: seatunnel-engine/seatunnel-engine-ui/src/views/jobs/detail.tsx, catch block of fetch() (clearInterval(timer)).
Problem: For a RUNNING job, a transient refresh rejection clears the 1 s duration interval. The page keeps the last good data and retries after 5 s, but the displayed duration is frozen until the next successful refresh recreates the interval.
Potential risk: Cosmetic only; the value self-corrects on the next successful refresh.
Best improvement: Leave timer running in the post-load branch of the catch (only clear it for the initial-load error card), or note it as accepted behavior.
Severity: Low
Raised by another reviewer: No

Issue 2 (Low, optional): an initial-load rejection is terminal until the user reloads

Location: detail.tsx, catch block, if (!job.jobStatus) branch.
Problem: When the very first getJobInfo rejects (for example a brief network blip), the full error card is shown and no retry is scheduled, while refresh failures after the first load do retry every 5 s.
Potential risk: The user has to reload the page. This is consistent with the agreed design ("full error card only before the first successful load"), so I am only flagging it for awareness.
Best improvement: Either keep as is, or add a small retry / "Retry" action on the error card in a follow-up.
Severity: Low
Raised by another reviewer: No

2. Code Quality Assessment

2.1 Coding Standards

Small and focused. The constant lives in RestConstant, comments explain the older-server fallback and the failover tolerance, Job.found? is documented in types.ts, and i18n keys exist in both en_US and zh_CN. No new core Java method was added, so there is no missing-comment issue.

2.2 Test Coverage and Test Stability

Test	Can it fail on a real bug?	Rating
`JobInfoServiceNullSafetyTest` unknown / running / finished marker tests	Yes: fail if the marker is dropped from the miss branch or leaks into running or finished payloads	Stable
`detail.spec.ts` missing, two status-less, one status-less recovery, initial rejection	Yes: each hits a distinct branch of `fetch()` with fake timers and call counts	Stable
`detail.spec.ts` refresh rejected / warn once per error streak	Yes: spy counts pin the dedupe guard and the reset	Stable
`test_diagnose_cli.py`, `test_sdk.py`	Yes: assert call counts, output text and the `TypeError` path; `time.sleep` is monkeypatched	Stable

Overall rating: Stable. No Thread.sleep, fixed ports or wall-clock dependence; timers are faked and now always restored in afterEach. I compared the modified existing tests against their previous versions: the renamed and extended Java tests only gained assertions, and no existing assertion was weakened.

2.3 Documentation Updates

docs/en and docs/zh are updated in lockstep for REST v1 (/job-info, /running-job), REST v2 (/job-info and the deprecated /running-job), the job-lifecycle troubleshooting row, and the Python SDK page. Wording matches the code (HTTP 200, found: false, older-server fallback).

3. Architectural Soundness

3.1 Elegance

Precise fix at the one place where "absent" is decided, with an additive contract.

3.2 Maintainability

One producer (JobInfoService.getJobInfoJson) and three consumers that follow the same rule: marker first, missing jobStatus as the legacy fallback.

3.3 Extensibility

If the project later decides to return 404, the found field can stay as a transitional signal; nothing here blocks that.

3.4 Historical-Version Compatibility

Old clients on a new engine and new clients on an old engine both keep working, as described in 1.2.

4. Issue Summary

Number	Issue	Location	Severity
1	Duration ticker stopped on a post-load refresh failure (optional)	`detail.tsx` `catch` in `fetch()`	Low
2	Initial-load rejection is terminal until reload (optional, matches agreed design)	`detail.tsx` `catch` in `fetch()`	Low

CI at this head 3578e25a4ff5: the apache-side Build check is a pointer to the fork run (wgzhao/seatunnel run 39608594369), which has completed with a failure. In that run the failing jobs are Run / changes (actions/setup-python error: Python 3.11.0 for x64 was not found for Ubuntu 26.04) and Run / Code style (seatunnel-ci-tools testCompile fails with a Lombok ExceptionInInitializerError); every other job was skipped because those two gate the pipeline. Both failures happen before any code from this PR is exercised (this PR does not touch seatunnel-ci-tools or the workflow files), so they look runner-image / environment related and not caused by this diff; this is my inference from the job logs. This is different from the LLMBooleanOutputTest failure seen on earlier heads. labeler and Notify test workflow passed. mergeStateStatus is BLOCKED (review required and required checks not green), and the branch is 7 commits behind dev without file overlap. Since the failure is environmental, please re-run only the failed jobs in the fork run (or retrigger once the runner issue is resolved upstream) rather than changing code; syncing with dev is optional.

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers - must be fixed
- The Build run for this head must finish green. The currently known red jobs (changes, Code style) are environmental and unrelated to the diff, so a targeted rerun of the failed jobs is the expected remedy. I will approve once it is green, as @wgzhao asked.
Recommended fixes - non-blocking
- Issue 1 and Issue 2 above (both optional, UI polish).

Overall assessment: the server change is minimal and correct, all three clients are consistent and tested with tests that can genuinely fail, EN/ZH docs match the code, and all earlier findings are closed. Alternative considered: returning 404 for unknown ids would change behavior for existing callers and remains a separate discussion in #12785. Thanks again for the careful and quick iteration.

SEZ9 · 2026-10-22T02:37:05Z

Thanks @wgzhao — moving vi.useRealTimers() into afterEach for every test in detail.spec.ts in 3578e25a4 addresses the fake-timer hygiene nit, and thanks for running vitest, prettier --check and vue-tsc locally.

Before I give the formal approval, I'd like to confirm the earlier review points against the actual changes rather than from the discussion alone. Could you point me to where each of these is handled on the new head (3578e25a4ff5 on top of bab2d089c)?

found: false documented for the deprecated v2 /running-job/:jobId and both v1 endpoints, not only /job-info/:jobId
CLI post-submit polling no longer stopping on a single status-less / non-dict body, and the "The id may be wrong" wording for an id obtained from /submit-job
UI fetch error path in detail.tsx and showing the looked-up job id in the missing-job state
JobsApi.job_exists handling of non-dict payloads

Once those are confirmed and CI is green on the new head, I'll approve. If any check stays red, please leave a short note here pointing to it so we can confirm on the thread that it is unrelated to this diff.

wgzhao · 2026-10-22T02:46:37Z

Thanks @SEZ9 — here are the exact locations on 3578e25a4 for each point.

1. found: false docs for v1 and the deprecated v2 /running-job/:jobId

v1 /hazelcast/rest/maps/job-info/:jobId: docs/en/engines/zeta/rest-api-v1.md:339-372 (ZH docs/zh/engines/zeta/rest-api-v1.md:359-390)
v1 /hazelcast/rest/maps/running-job/:jobId: docs/en/engines/zeta/rest-api-v1.md:454-464 (ZH docs/zh/engines/zeta/rest-api-v1.md:449-459)
v2 deprecated /running-job/:jobId: docs/en/engines/zeta/rest-api-v2.md:770-780 (ZH docs/zh/engines/zeta/rest-api-v2.md:742-752); the full marker description lives in the v2 /job-info/:jobId section at docs/en/engines/zeta/rest-api-v2.md:687-698
Python SDK page: docs/en/engines/zeta/python-sdk.md:72 (ZH :73)

2. CLI polling and the "id may be wrong" wording

Post-submit polling enters _poll_job_status at seatunnel-cli/seatunnel_cli/cli.py:1358; implementation :1360-1400. It stops immediately on found: false (:1375), tolerates one status-less/non-dict 200 (:1386), and on a repeat prints the response-shape / master-failover message (:1393).
"The id may be wrong" now appears only in run_diagnose's HTTP 404 branch (:1657), for an id the user supplied. The post-submit path no longer uses that wording. run_diagnose checks found first at :1687-1688.

3. UI error path and missing-state job id

detail.tsx:95-155: found === false switches to the missing state (:98), one status-less 200 is tolerated and a second switches to missing (:106-120), and the catch distinguishes an initial-load error from a post-load refresh rejection (:139-154).
The missing state renders the requested id via t('detail.notFound.description', { jobId }) at :616.

4. SDK non-dict handling

tools/seatunnel-python-sdk/seatunnel/endpoints/jobs.py:90-105: job_exists raises TypeError for a non-dictionary/non-JSON body (:100) before checking found (:104-105).

On CI: run 39608594369 skipped 53 jobs and failed only two pre-code jobs — changes (actions/setup-python@v5: Python 3.11.0/x64 not available for Ubuntu 26.04, since ubuntu-latest migrated to 26.04) and Code style (seatunnel-ci-tools testCompile: Lombok ExceptionInInitializerError on the new runner image). Neither exercises this diff, and a plain rerun on the same image will likely reproduce them; the durable fix looks CI-side (pin the runner image or update setup-python/Lombok), not in this PR. As suggested, the failed jobs should be re-run rather than changing code.

DanielLeens · 2026-10-22T09:57:50Z

Thanks for the detailed location map, @wgzhao, and for the independent recheck, @SEZ9. There is no new head in this exchange, so my earlier source-level conclusion remains unchanged; I am not marking @SEZ9's independent threads resolved on their behalf.

I also narrowed the current CI failures instead of repeating my earlier generic rerun advice:

Code style / Check code specification fails during seatunnel-ci-tools test compilation, in Lombok initialization: NoSuchFieldException: com.sun.tools.javac.code.TypeTag :: UNKNOWN. It is not a Spotless diff in your changed files. The job runs on ubuntu-26.04 and the shared job has no explicit supported-JDK setup.
changes / setup-python fails because Python 3.11.0 x64 is unavailable for Ubuntu 26.04.

Latest dev still uses ubuntu-latest, has no JDK setup in code-style, and pins Python 3.11.0 in changes; I found no merged correction for either gate. So syncing current dev alone is not a fix, and a blind rerun is not a demonstrated remedy for these toolchain/image mismatches. The minimal next step is a focused shared-CI correction by the workflow owners: select a project-supported JDK for the specification check and a compatible Python/runner combination. Once that lands, sync and rerun the affected jobs. Please keep unrelated Lombok/dependency changes out of this PR.

The source review can stay focused on the endpoint/client changes; final merge still needs the actual required Build and @SEZ9's formal recheck. Thanks for making the review pointers easy to follow.

SEZ9 · 2026-10-23T02:09:40Z

Thanks for the precise location map on 3578e25a4, @wgzhao — that makes the recheck much easier to follow.

F1 / F3 (found: false docs) — The pointers for both v1 endpoints, the deprecated v2 /running-job/:jobId, the ZH counterparts, and the Python SDK page cover every endpoint I flagged. I'll confirm the wording against the diff on 3578e25a4 before closing these.

F2 / F4 (CLI polling) — The _poll_job_status behaviour you describe (stop immediately on found: false, tolerate a single status-less / non-dict 200, and only emit the response-shape / master-failover message on a repeat) is exactly what I was asking for. I'll verify it in the diff and then mark these resolved.

F5 ("The id may be wrong") — Moving that wording into run_diagnose's HTTP 404 branch sounds like the right place, since there the id genuinely comes from the user rather than from /submit-job. Your comment appears to be cut off right after "HTTP 404 branch (" — if there was more to that point (e.g. the line reference), please paste it so I can check it.

Still open — your reply didn't cover these three; a pointer on 3578e25a4 or a short note on how you'd like to handle them would be great:

F6 — UI fetch error path in seatunnel-engine/seatunnel-engine-ui/src/views/jobs/detail.tsx: a rejected getJobInfo should surface an error state rather than leaving the detail page blank, so the new missing-job state can actually be reached.
F7 — JobsApi.job_exists in tools/seatunnel-python-sdk/seatunnel/endpoints/jobs.py still returns False for any non-dict payload; mirroring the CLI distinction between "server said found: false" and "response wasn't the expected shape" would avoid conflating the two.
F8 — the UI "Job not found" state should include the looked-up job id so users can see what was actually queried.

Once F6–F8 are addressed (or you've explained why they should stay as-is), I'll do the formal recheck of all eight points against the diff.

wgzhao · 2026-10-23T05:08:31Z

Thanks @SEZ9 — it looks like my previous comment was truncated on your side after "HTTP 404 branch (". Re-pasting the missing pieces, plus the F6–F8 pointers you asked for (all on head 3578e25a4).

F5 — seatunnel-cli/seatunnel_cli/cli.py:1657 is the run_diagnose HTTP 404 branch where the "id may be wrong" wording now lives.
F6 — seatunnel-engine/seatunnel-engine-ui/src/views/jobs/detail.tsx:139-154: the catch in fetch() shows the full error card for an initial-load failure, and after a successful load keeps the populated page (one warning per error streak) while retrying on the next tick. Covered by detail.spec.ts cases "should show an error result when getJobInfo rejects" and "should keep the loaded job when a refresh is rejected".
F7 — tools/seatunnel-python-sdk/seatunnel/endpoints/jobs.py:90-105: job_exists raises TypeError for a non-dictionary/non-JSON body at :100, and only then checks found at :104-105, so "server said absent" and "unexpected response shape" are no longer conflated. Covered by test_sdk.py.
F8 — detail.tsx:616 renders the missing-state description via t('detail.notFound.description', { jobId }); detail.spec.ts asserts the route id (123456789) is shown.

F6–F8 were already included in my previous reply as points 3 and 4 — the truncation just hid them. Nothing else to change on those three; let me know if you want any additional pointer.

DanielLeens · 2026-10-23T07:31:35Z

Thanks for the detailed location map, @wgzhao, and for the follow-up pointers for F5 to F8, @SEZ9 and @wgzhao. There is still no new head here (it remains 3578e25a4ff5), so my earlier source-level conclusion is unchanged, and I am not closing @SEZ9's independent threads on their behalf.

Two CI/mergeability facts have changed since my last note:

The shared CI gate I flagged is now fixed upstream: #12964 merged into dev (merge 14a27ac26a0a). It pins JDK 17 in the code-style job and relaxes the exact Python 3.11.0 pin in the changes job, which are exactly the two failures in your fork run (Code style and changes). Those failures did not come from your diff.
The PR now reports mergeable_state: dirty, so dev has moved in a way that conflicts with this branch. This is a real blocker separate from CI: please sync the latest dev and resolve the conflicts (keeping the resolution limited to the files this PR already touches).

Minimal next step: sync latest dev and resolve the conflict, push, and let the Build run on the new head so it includes #12964. That fresh Build, plus @SEZ9's formal recheck, are the remaining gates; please do not add unrelated patches to chase CI. Once the new head is pushed I will recheck it.

This is a static reply; nothing was executed locally.

wgzhao · 2026-10-23T07:47:12Z

Thanks — synced onto dev at 14a27ac2 (#12964) and resolved the conflicts (resolution limited to the files this PR already touches). New head: 875a7ff788; mergeable_state is clean again. The fresh Build run 39919297925 already has both gates green: changes and Code style both passed. Letting the full Build finish now.

DanielLeens

Thank you @wgzhao for syncing onto dev (14a27ac, which includes #12964) and resolving the conflict so quickly. The head moved from 3578e25a4ff5 (my last reviewed commit) to 875a7ff788e9. I re-read the whole PR from scratch on the new head rather than only the delta. This is a pure rebase: the lines this PR adds and removes are byte-identical to 3578e25a4, and the only differences are context lines in the REST docs that changed on dev in the meantime (finishedTime became finishTime, and the old "When we can't get the job info" wording was replaced). Daniel's earlier source-level conclusion therefore stands: no source-level blocker. I also read @SEZ9's threads and your latest replies; I am not closing @SEZ9's threads on their behalf.

What Problem Does This PR Solve?

Pain point: GET /job-info/{jobId} answers HTTP 200 with only jobId when the id is neither running nor in finished-job history. Status-code-only checks treat a missing job as healthy, the Web UI parses undefined timestamps and keeps polling, and --diagnose can print "Nothing to report" (refs #12785).
Fix approach: Keep HTTP 200 and every successful payload unchanged. Add an additive found: false marker on the unknown-id branch of JobInfoService, and teach the Web UI, Python SDK and CLI to prefer the marker with a "no jobStatus" fallback for older engines.
One sentence: An additive, backward-compatible unknown-job marker with consistent client handling.
Before / after: GET /job-info/999 for an unknown id used to return {"jobId":"999"}; it now returns {"jobId":"999","found":false} (still HTTP 200), and known jobs return exactly what they returned before.

1. Code Change Review

1.1 Core Logic Analysis

Server change (the only wire change), JobInfoService.java:111-117:

// before
return new JsonObject().add(RestConstant.JOB_ID, jobId.toString());
// after
return new JsonObject()
        .add(RestConstant.JOB_ID, jobId.toString())
        .add(RestConstant.FOUND, false);

Runtime path (re-verified on this head):

Jetty v2 /job-info/:jobId and deprecated /running-job/:jobId
Hazelcast v1 /hazelcast/rest/maps/{job-info,running-job}/{id}
  -> JobInfoService.getJobInfoJson(Long)
       running map hit   -> convertToJson(...)   (unchanged, no marker)
       finished map hit  -> finished payload     (unchanged, no marker)
       both miss         -> {jobId, found:false} (only changed branch)
  -> HTTP 200
Clients:
  Web UI  detail.tsx fetch()      found===false -> missing; no jobStatus -> tolerate one, then missing; rejection -> error card (first load) or one warning per streak (after load)
  Python  JobsApi.job_exists()    "found" in details -> bool(found); else "jobStatus" in details; non-dict -> TypeError
  CLI     _poll_job_status()      found is False -> stop; status-less -> tolerate one, then stop with a distinct response-shape message
  CLI     run_diagnose()          found is False -> "marked it as unknown", exit 1; status-less fallback kept

Does the normal user path reach the changed logic? The successful (running or finished) lookups never take the changed branch, so they cannot gain the field. The unknown/expired-id path is the one that reaches it, and that is exactly the reported pain point.

Status of my earlier findings and the findings from @SEZ9 that I have checked against this head:

Earlier finding	Status on `875a7ff788e9`
Docs for v1 `/job-info` and `/running-job` and v2 deprecated `/running-job` still showed `{"jobId": ""}`	Fixed (EN and ZH, each pointing at the v2 description; the conflict resolution kept them)
CLI polling gave up on the first status-less or non-dict body	Fixed (one tolerated, a repeat prints a distinct message)
"The id may be wrong" for an id the CLI itself received	Fixed (wording only remains in the diagnose 404 branch)
UI `fetch` had no rejection path	Fixed
`job_exists` reported a non-JSON body as "does not exist"	Fixed (`TypeError`)
Not-found card did not show the job id	Fixed
Refresh-warning test could not fail; test-name drift; fake timers restored only on success path	Fixed in earlier heads

1.2 Compatibility Impact

Fully compatible. HTTP status and all successful payloads are unchanged; the only wire change is one extra boolean on the unknown-id body. New clients fall back to the jobStatus check against older engines, and old clients ignore the extra key. No option, default, serialization, checkpoint or savepoint state change; the 404 question stays open in #12785 as agreed.

1.3 Performance / Side-Effect Analysis

Nothing on a hot path: one boolean on the miss branch. The UI stops polling a missing job and the CLI stops polling an unknown id (previously up to 150 polls). No new lookups, threads or retained state. The UI and CLI tolerate a single status-less response, which bounds extra polling to one additional tick.

1.4 Error Handling and Logging

I found no formal issue that needs a change before merge. Two optional UI observations from my previous review remain, unchanged and non-blocking:

Issue 1 (optional)
- Location: seatunnel-engine/seatunnel-engine-ui/src/views/jobs/detail.tsx (catch in fetch())
- Problem: on a refresh rejection after a successful load, clearInterval(timer) stops the 1-second duration ticker, and it is only restarted by the next successful poll.
- Potential risk: the displayed duration freezes during an outage window; cosmetic only.
- Best improvement: leave as is, or restart the ticker in the retry path in a follow-up.
- Severity: Low
- Raised by another reviewer: No
Issue 2 (optional)
- Location: seatunnel-engine/seatunnel-engine-ui/src/views/jobs/detail.tsx (catch in fetch())
- Problem: an initial-load rejection is terminal until the page is reloaded.
- Potential risk: users must reload manually; this matches the design agreed in the thread ("full error card only before the first successful load").
- Best improvement: optionally add a Retry action on the error card in a follow-up.
- Severity: Low
- Raised by another reviewer: No

2. Code Quality Assessment

2.1 Coding Standards

Small and focused. The constant lives in RestConstant, comments explain the older-server fallback and the failover tolerance, Job.found? is documented in types.ts, and i18n keys exist in both en_US and zh_CN. No new core Java method was added, so no missing-comment issue.

2.2 Test Coverage and Test Stability

Test	Can it fail on a real bug?	Rating
`JobInfoServiceNullSafetyTest` unknown / running / finished marker tests	Yes: fail if the marker is dropped from the miss branch or leaks into running or finished payloads	Stable
`detail.spec.ts` missing, two status-less, one status-less recovery, initial rejection, refresh rejection and recovery	Yes: each hits a distinct branch of `fetch()` with fake timers, call counts and a `warning` spy; timers are restored in `afterEach`	Stable
`test_diagnose_cli.py`, `test_sdk.py`	Yes: assert call counts, output text and the `TypeError` path; `time.sleep` is monkeypatched	Stable

Overall rating: Stable. No Thread.sleep, fixed ports or wall-clock dependence. The renamed and extended Java tests only gained assertions; no existing assertion was weakened. This is a static code-reading assessment; I did not run the tests locally.

2.3 Documentation Updates

docs/en and docs/zh are updated in lockstep for REST v1 (/job-info, /running-job), REST v2 (/job-info and the deprecated /running-job), the job-lifecycle troubleshooting row, and the Python SDK page. On this head the old "response contains only the requested jobId" wording from dev is no longer present in the touched sections, so the rebase did not leave a contradiction.

3. Architectural Soundness

3.1 Elegance

Precise fix at the one place where "absent" is decided, with an additive contract.

3.2 Maintainability

One producer (JobInfoService.getJobInfoJson) and three consumers that follow the same rule: marker first, missing jobStatus as the legacy fallback.

3.3 Extensibility

If the project later decides to return 404, found can stay as a transitional signal; nothing here blocks that.

3.4 Historical-Version Compatibility

Old clients on a new engine and new clients on an old engine both keep working, as described in 1.2.

4. Issue Summary

Number	Issue	Location	Severity
1	Duration ticker stopped on a post-load refresh failure (optional)	`detail.tsx` `catch` in `fetch()`	Low
2	Initial-load rejection is terminal until reload (optional, matches agreed design)	`detail.tsx` `catch` in `fetch()`	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers - must be fixed
- None at the source level. The only remaining gate is that the Build for this head finishes green, plus @SEZ9's formal recheck.
Recommended fixes - non-blocking
- Issue 1 and Issue 2 above (optional UI polish).

CI / merge state (facts only): the head is 875a7ff788e9 and mergeable is clean again after your sync. The apache-side Build check is a pointer to the fork run (wgzhao/seatunnel run 39919297925), which was still in progress when I looked: changes, Code style, License header, Dead links, Sanity check results, Build SeaTunnel UI, the unit-test jobs (Java 11/17, Ubuntu and Windows), engine-v2-it (11 and 17) and engine-k8s-it had completed successfully, with the remaining integration jobs still running or queued. The two earlier environmental failures (changes / setup-python and Code style on Ubuntu 26.04) are gone on this head, consistent with #12964 having landed. The overall merge state is BLOCKED only because a review approval and the required checks are still pending. No action is needed from you unless a lane turns red; if one does, please leave a short note so we can confirm on the thread whether it relates to this diff.

Overall assessment: the server change is minimal and correct, the three clients are consistent and covered by tests that can genuinely fail, and EN/ZH docs match the code. Alternative considered: returning 404 for unknown ids would change behavior for existing callers and remains a separate discussion in #12785. Thanks again for the careful iteration.

SEZ9 · 2026-10-24T02:08:31Z

Thanks @wgzhao — the F5–F7 pointers came through this time, but the message is cut off again on my side at the end of the F8 item, right after detail.sp. Could you post the rest of the F8 item as a separate short comment?

On what did arrive, all against 3578e25a4:

F5 — Understood that the "id may be wrong" wording now lives in the run_diagnose HTTP 404 branch at seatunnel-cli/seatunnel_cli/cli.py:1657. For a user-supplied id in diagnose that hint is reasonable; the concern was only about saying it for an id the CLI itself just received from /submit-job. I'll check the post-submit polling path on 3578e25a4 and close F5 if that wording is gone there.
F6 — The split you describe at seatunnel-engine/seatunnel-engine-ui/src/views/jobs/detail.tsx:139-154 (full error card on initial-load failure; keep the populated page and retry with one warning per error streak after a successful load) is exactly the behaviour I was after, and the two detail.spec.ts cases you named sound like they cover both branches. I'll verify against the diff and mark it resolved once I've looked.
F7 — Raising TypeError for a non-dict/non-JSON body at :100 before checking found at :104-105 in tools/seatunnel-python-sdk/seatunnel/endpoints/jobs.py:90-105 is the right separation of "server said absent" from "unexpected response shape". Same here: I'll confirm in the diff along with the test_sdk.py coverage.
F8 — t('detail.notFound.description', { jobId }) at detail.tsx:616 addresses the ask; I'll close it once I can see the spec reference from the truncated part and check the diff.

Remaining asks before I sign off:

The missing tail of your F8 item.
A quick status on F1/F3 (documenting found: false for the deprecated v2 /running-job/:jobId and both v1 endpoints, not only v2 /job-info/:jobId) and F2/F4 (post-submit polling aborting on the first status-less / non-dict body and reporting a non-object payload as "job is not known to the cluster"). Your message doesn't mention them — are they already handled on 3578e25a4, or still pending? Pointers in the same style as above would be perfect.

wgzhao · 2026-10-24T02:18:25Z

F8 tail (the part that got cut): detail.spec.ts asserts that the not-found description renders the route job id (123456789), so the looked-up id is covered by a test. That completes F8.

wgzhao · 2026-10-24T02:18:28Z

F1/F3 (found:false docs) are on 3578e25:

v1 /job-info: docs/en/engines/zeta/rest-api-v1.md:339-372 (ZH :359-390)
v1 /running-job: docs/en/engines/zeta/rest-api-v1.md:454-464 (ZH :449-459)
v2 deprecated /running-job: docs/en/engines/zeta/rest-api-v2.md:770-780 (ZH :742-752)

wgzhao · 2026-10-24T02:18:32Z

F2/F4 (CLI post-submit polling) are in seatunnel-cli/seatunnel_cli/cli.py:

:1360-1400 _poll_job_status
:1375 stops on found:false
:1386 tolerates one status-less/non-dict 200
:1393 a repeat prints the distinct response-shape message (never 'job is not known')
Post-submit entry: :1358.

wgzhao · 2026-10-24T02:18:36Z

The Build on the new head 875a7ff is fully green: fork run 39919297925, 85/85 jobs. The rebase is content-identical to 3578e25 per Daniel's check. Could you run the formal recheck on this head?

DanielLeens · 2026-10-24T10:48:13Z

Thanks @wgzhao for the CI update and for answering @SEZ9's F1 to F8 questions with exact pointers.

I confirmed the CI fact on the unchanged head 875a7ff788e9: the fork run wgzhao/seatunnel 39919297925 has completed with conclusion success for this exact SHA, with no failed jobs. In my review of this head I found no source-level blocker and listed this Build as the only remaining gate from my side, so that gate is now met and my conclusion on 875a7ff788e9 is ready to merge. The two Low UI items in that review remain optional.

The formal recheck you asked for in your last line is @SEZ9's own pass on their F1 to F8 threads; I am not closing those on their behalf. The final merge still needs an approval from a committer with write access. No further action is needed from you unless a new review comment comes in.

SEZ9 · 2026-10-25T03:35:43Z

Thanks @wgzhao for the pointers on 3578e25 / 875a7ff.

F1 / F3 (found:false docs), F2 / F4 (CLI post-submit polling), F8 (UI not-found shows the job id) — The locations you listed (the v1 /job-info and /running-job sections in rest-api-v1.md and the deprecated v2 /running-job section in rest-api-v2.md, EN + ZH; _poll_job_status in cli.py; the detail.spec.ts assertion that the not-found description renders the route job id 123456789) are exactly what I need. I'll check those spots in the diff on 875a7ff and resolve the threads once I've confirmed the content there, rather than closing them from the description alone.

Still open from my earlier pass:

F5 — Is the post-submit "The id may be wrong" wording still present for the id the CLI itself received from /submit-job? If it was reworded alongside F2/F4, a pointer in cli.py would be enough.
F6 — Does the UI fetch in detail.tsx now have an error path for a rejected getJobInfo, so the page isn't left blank? A pointer, or a note if you'd prefer to keep this as a follow-up, is fine.
F7 — Has JobsApi.job_exists in jobs.py been adjusted so a non-dict/non-JSON payload is not reported as "job does not exist"? Same ask: pointer or explicit deferral.

I've noted the green fork run 39919297925 on 875a7ff. Once I've verified the pointers above and F5–F7 are confirmed or explicitly deferred, I'll have nothing further within this scope; the final merge approval remains with a committer with write access.

wgzhao · 2026-10-25T07:18:14Z

F5: only cli.py:1659 has the "id may be wrong" text (run_diagnose 404 branch). Post-submit polling (_poll_job_status, cli.py:1360) does not contain it.
F6: detail.tsx:139-154 - initial-load rejection shows the error card (:143); post-load rejection keeps the page, warns once (:151) and retries (:153). Tests: detail.spec.ts (two reject cases).
F7: jobs.py:90-106 - non-dict/non-JSON raises TypeError (:100); found is checked at :104. test_sdk.py covers it.

DanielLeens · 2026-10-25T10:50:31Z

Thanks @wgzhao for the precise pointers, and thanks @SEZ9 for keeping the final checks explicit. I checked the unchanged head locally: the CLI's only "id may be wrong" occurrence is in the diagnose branch at cli.py:1659, not _poll_job_status; detail.tsx:139-154 handles initial-load rejection with an error card and refresh rejection with a warning/retry; tools/seatunnel-python-sdk/seatunnel/endpoints/jobs.py:98-106 raises on a non-dict payload before checking found. Those pointers answer the concrete F5-F7 questions without a new commit. My previous technical conclusion remains unchanged. I will leave @SEZ9 to verify and resolve their own threads; that final pass and write-capable approval are not being marked complete by this reply.

…e#12785)

DanielLeens

Thank you @wgzhao for syncing again. The head moved from 875a7ff788e9 (my last reviewed commit) to 4cfc9b02bfee4a326e042b79e53b7bcfe1fad857, and everything I re-checked still holds. This is a static source review; CI is the runtime evidence.

I re-read the PR from scratch on the new head instead of only the delta. The branch was rebased onto the current dev (c8e7896157c6, so behind_by is 0) and the two commits are 6847d86c (the feature) and 4cfc9b02 (fake-timer hygiene). I compared the added and removed lines of the whole PR against the previous head (14a27ac2...875a7ff788e9), and the 695 changed lines are identical. Only the context moved. My earlier conclusion therefore stands: no source-level blocker.

What Problem Does This PR Solve?

User pain point: GET /job-info/{jobId} answers HTTP 200 with only jobId when the id is neither running nor in finished-job history. Status-code-only checks treat a missing job as healthy, the Web UI parses undefined timestamps and keeps polling, and --diagnose can print "Nothing to report" (refs #12785).
Fix approach: Keep HTTP 200 and every successful payload unchanged. Add an additive found: false marker on the unknown-id branch of JobInfoService, and make the Web UI, Python SDK and CLI prefer the marker with a "no jobStatus" fallback for older engines.
One sentence: An additive, backward-compatible unknown-job marker with consistent client handling.

1. Code Change Review

1.1 Core Logic Analysis

// before (JobInfoService.getJobInfoJson(Long), final else branch)
return new JsonObject().add(RestConstant.JOB_ID, jobId.toString());
// after
return new JsonObject()
        .add(RestConstant.JOB_ID, jobId.toString())
        .add(RestConstant.FOUND, false);

Runtime path:

Jetty v2 /job-info/:jobId and deprecated /running-job/:jobId, v1 /hazelcast/rest/maps/{job-info,running-job}
  -> JobInfoService.getJobInfoJson(Long)
       IMAP_RUNNING_JOB_INFO hit   -> running payload   (no marker)
       IMAP_FINISHED_JOB_STATE hit -> finished payload  (no marker)
       both miss                   -> {jobId, found:false}   <- the only wire change
Clients
  detail.tsx fetch(): found===false -> not-found card; no jobStatus twice -> not-found card (older engines)
  cli.py _poll_job_status / run_diagnose: found===false -> stop with a clear message
  JobsApi.job_exists: "found" if present, else "jobStatus" in details

Key findings:

The marker is added only in the last else, after both state maps miss, so running and finished responses cannot gain the field. This is a normal-path change for lookups of unknown or expired ids, and it is not touched by checkpoint, restore or scheduling logic.
The new running-branch test and the two assertNull(result.get(RestConstant.FOUND)) assertions pin that contract.
The UI keeps the last good page on a transient refresh failure, warns once per error streak and retries on the next 5 s tick. The full error card is used only before the first successful load.
The CLI tolerates exactly one status-less 200 (possible master-failover window on older servers) and a repeat gets a distinct response-shape message instead of claiming the id is wrong.

Correctness: no defect found.

1.2 Compatibility Impact

Fully compatible. HTTP status and all existing fields are unchanged. Only the previously degenerate {jobId} body gains one extra key, which existing clients ignore. New clients fall back to "no jobStatus" against older engines. No config option, default, SPI or serialization format changes, and nothing touches checkpoint or savepoint state.

1.3 Performance / Side-Effect Analysis

One extra boolean in a response that is already tiny and only built on the unknown-id path. The UI and CLI poll intervals are unchanged (5 s and 2 s) and both stop on a definitive unknown result, which slightly reduces needless polling. No new threads, queues or retained state.

1.4 Error Handling and Logging

Non-dict and non-string status values are handled in the CLI and SDK (TypeError with a clear message in job_exists). The UI catches rejections and surfaces them instead of leaving an unhandled promise. No sensitive data is logged. No new issues found.

2. Code Quality Assessment

2.1 Coding Standards

Follows the existing style. The new RestConstant.FOUND is consistent with its neighbours, the new SDK method and the UI type carry short, accurate comments, and the CLI failover tolerance is explained where it is implemented. No missing documentation on core methods.

2.2 Test Coverage and Test Stability

Java: JobInfoServiceNullSafetyTest covers unknown (marker present, no jobStatus), running (marker absent) and finished (marker absent) payloads.
Python: CLI tests cover the marker, the older-server status-less path (two calls) and recovery after one status-less response; SDK tests cover job_exists.
UI: detail.spec.ts covers the marker, one-status-less recovery, a repeated status-less response, the initial-load rejection, the post-load rejection and its single-warning streak behavior, plus the job id shown on the not-found card. 4cfc9b02 moves vi.useRealTimers() into afterEach, so a failing assertion can no longer leak fake timers into the next test.
Flaky-test stability analysis: the Python tests stub time.sleep and request_json, so they are deterministic. The UI tests use fake timers with explicit advancement and a mocked useMessage().warning, with real timers restored in afterEach. No real network, wall-clock or ordering dependency. Rating: Stable.

2.3 Documentation Updates

EN and ZH pages are updated for v1 /job-info and /running-job, v2 /job-info and the deprecated v2 /running-job, the job lifecycle page and the Python SDK page, with matching content on both sides.

3. Architectural Soundness

3.1 Precise fix / Temporary workaround / Long-term solution

A precise fix at the single place that builds the ambiguous body, with additive clients. It is not a workaround. Changing the status code to 404 would be cleaner in isolation but would break existing consumers, so the agreed additive marker in #12785 is the right trade-off.

3.2 Maintainability

Small, well-localized server change; the client fallbacks are explicit and can be removed once older engines age out.

3.3 Extensibility

The same marker convention can be reused by other REST handlers that currently return degenerate bodies.

3.4 Historical Compatibility

Older servers and older clients keep working, as analyzed in 1.2.

4. Issue Summary

#	Severity	Location	Summary	Raised by another reviewer
-	-	-	No source-level issue found at this head	-

@SEZ9's earlier points (F1 to F8: v1 and deprecated v2 docs, CLI post-submit polling, the diagnose wording, UI refresh handling, SDK non-dict handling, not-found card job id) are all addressed in the code, as I verified against the current head. I am not closing @SEZ9's threads on their behalf, and a formal approval from them is still welcome.

5. Merge Recommendation

Conclusion: Ready to merge

Blockers: none at the source level.

Gate status (not a code issue): the fork Build run for this head, wgzhao/seatunnel run 40668142760, was still queued when I checked, so there is no green or red result for this exact SHA yet. The previous head 875a7ff788e9 passed the full fork Build (85/85 jobs) and this rebase is content-identical for the PR's own lines, which is supporting evidence only and not a substitute. Please merge only after this run finishes green. If an unrelated shared-infrastructure job fails, a rerun of just that job is the right first step.

Recommended fixes: none.

Overall assessment: a small, additive, well-tested and well-documented fix that closes a real "missing job looks healthy" trap across the server, Web UI, SDK and CLI without breaking any existing consumer.

Alternatives considered: returning HTTP 404 (rejected as breaking for existing clients) or keeping client-side jobStatus sniffing only (rejected as fragile and still invisible to status-code checks).

DanielLeens · 2026-10-29T17:12:58Z

The previously queued Build has completed with a failure. Its only failed job is doris-connector-it (11), run 40668142760/job 122749217860: DorisSchemaChangeIT fails before its scenario because apache/doris:doris-all-in-one-2.1.0 does not open its mapped ports within the startup window. This is a container-startup signal, not a failing found:false assertion. Latest dev's Doris fixture history contains no corresponding newer fix beyond this head. Rerun that failed job and inspect the container startup log if it repeats; a sync alone is not an established cure, and no unrelated Doris patch is requested in this job-info PR.

Checked 2026-10-29T17:12:53+00:00; head 4cfc9b02bfee4a326e042b79e53b7bcfe1fad857. Signal pull request update = SUCCESS; Lint = SUCCESS; Test with Python 3.9 = SUCCESS; Test with Python 3.13 = SUCCESS; Test with Python 3.10 = SUCCESS; Test with Python 3.13 = SUCCESS; Build = FAILURE

Comparison: base dev, head 4cfc9b02bfee4a326e042b79e53b7bcfe1fad857, diverged, ahead 2 / behind 17; GitHub mergeable state blocked. Behind/diverged alone is recorded metadata and does not add a sync request.

Daniel has READ repository permission. This is a discussion/CI clarification, not a new complete source review, an effective APPROVED event or dismissal of historical requested changes. A write-capable maintainer must perform any required final approval/dismissal. Thanks for keeping the follow-up focused.

SEZ9 · 2026-10-30T02:28:41Z

Thanks for the CI follow-up. On head 4cfc9b02bfee4a326e042b79e53b7bcfe1fad857, Signal pull request update, Lint, and the Python 3.9 / 3.10 / 3.13 test jobs are SUCCESS; the Build failure (122810157253) is limited to doris-connector-it (11), run 40668142760 / job 122749217860, where DorisSchemaChangeIT fails because the Doris container does not open its mapped ports within the startup window. That looks like a container-startup issue rather than a found: false assertion failure. Please rerun that job; if it repeats, please share the container startup log. No unrelated Doris change is needed in this PR.

From the earlier review, these points still appear open (please point me to the commit if any are already addressed):

Docs for found: false – the payload is also returned by the deprecated /running-job/:jobId endpoint and the v1 endpoints, but only the v2 /job-info section of docs/en/engines/zeta/rest-api-v2.md was updated. Please update the other entries to match.
CLI post-submit polling (seatunnel-cli/seatunnel_cli/cli.py) – polling currently aborts on the first status-less or non-dict body and reports "job is not known to the cluster". Please only treat an explicit found: false as "not known", keep retrying on ambiguous bodies, or surface a distinct message for a non-JSON response.
Polling message wording – "The id may be wrong" is misleading when the id came straight from /submit-job; please reword.
UI fetch error path (seatunnel-engine/seatunnel-engine-ui/src/views/jobs/detail.tsx) – a rejected getJobInfo still leaves the page blank, so the new missing-job state never renders. Please add a catch that sets an error/not-found state.
UI "Job not found" text – please include the job id that was looked up.
JobsApi.job_exists (tools/seatunnel-python-sdk/seatunnel/endpoints/jobs.py) – a non-dict payload currently returns False; please distinguish a non-JSON response from an explicit found: false.

Once these are pushed and the Doris job has been rerun, I'll take another pass.

DanielLeens · 2026-10-30T03:26:45Z

Thanks @SEZ9. +1 on keeping the Doris startup failure separate from the found:false behavior. I rechecked the exact-head locations behind your six points: both v1 entries and the deprecated v2 running-job entry document found:false; CLI cli.py:1375-1396 only calls an explicit found:false missing, while repeated status-less bodies produce a response-shape/master-failover diagnostic; the post-submit message no longer blames a user-supplied ID. UI detail.tsx:139-153 handles fetch rejection and :609-618 includes jobId in its error/not-found card. SDK endpoints/jobs.py:98-106 raises TypeError for a non-dict response. These were already included in my complete source review, so I do not see an outstanding source blocker from those six points. The same Doris container-startup job remains red; current dev adds no matching fix for that fixture. Please rerun that failed job only and retain the container startup log if it repeats. A simple sync is not an established cure and no unrelated Doris patch is requested here.

Snapshot 2026-10-30T03:23:54.151954+00:00: head 4cfc9b02bfee4a326e042b79e53b7bcfe1fad857; Signal pull request update = SUCCESS; Lint = SUCCESS; Test with Python 3.9 = SUCCESS; Test with Python 3.13 = SUCCESS; Test with Python 3.10 = SUCCESS; Test with Python 3.13 = SUCCESS; Build = FAILURE.

Base dev; comparison diverged, ahead 2, behind 20; merge state blocked. Behind/diverged alone is recorded metadata and does not add a sync request. This is an unchanged-head discussion/CI clarification, not a new complete source review, an effective approval or a dismissal of historical requested changes.

wgzhao · 2026-10-30T05:51:46Z

Thanks @SEZ9 and @DanielLeens. I reran the only failed job, doris-connector-it (11) (run 40668142760, attempt 2, in progress). That job is an existing container-based Doris integration suite; it does not exercise the job-info endpoint, so its failure is unrelated to this diff. I'll report the result here.

The head is unchanged at 4cfc9b02b, and all six points were addressed in commit 6847d86cc (the feature commit; 4cfc9b02 only moves vi.useRealTimers() into afterEach). Pointers on the current head:

found: false docs — v1 /hazelcast/rest/maps/job-info/:jobId: docs/en/engines/zeta/rest-api-v1.md:402-439 (ZH :422-457); v1 /hazelcast/rest/maps/running-job/:jobId: rest-api-v1.md:520-533 (ZH :514-527); v2 deprecated /running-job/:jobId: docs/en/engines/zeta/rest-api-v2.md:809-822 (ZH :796-809); v2 /job-info/:jobId: rest-api-v2.md:678-732 (ZH :669-719).
CLI post-submit polling — seatunnel-cli/seatunnel_cli/cli.py:1360-1400: polling stops only on an explicit found: false (:1375); one status-less or non-dict 200 is tolerated (:1388-1389), and a repeat prints a distinct response-shape / master-failover message (:1392-1395) instead of treating the id as unknown.
Polling wording — the only "id may be wrong" sentence is in run_diagnose's HTTP 404 branch (cli.py:1659), where the id is user-supplied; _poll_job_status does not contain it.
UI fetch error path — seatunnel-engine/seatunnel-engine-ui/src/views/jobs/detail.tsx:95-155: an initial-load rejection sets loadError and renders the error card (:143-146); a post-load rejection keeps the loaded page, warns once per error streak (:149-152) and retries (:153). Covered by detail.spec.ts cases "should show an error result when getJobInfo rejects" and "should keep the loaded job when a refresh is rejected".
UI not-found text — detail.tsx:616 passes { jobId } to detail.notFound.description; detail.spec.ts:187-188 asserts that the route id 123456789 is rendered.
JobsApi.job_exists — tools/seatunnel-python-sdk/seatunnel/endpoints/jobs.py:90-106: a non-dict / non-JSON body raises TypeError (:100) before found is checked (:104-105); test_sdk.py:114-159 covers the explicit marker, the older-server fallback, the status-less case and the non-JSON case.

wgzhao · 2026-10-30T07:38:23Z

Confirmed green: the rerun of fork run 40668142760, attempt 2 completed with conclusion success, and the PR's Build check is now pass. The apache/doris:doris-all-in-one-2.1.0 container that timed out on the first attempt started normally this time, so the previous failure was environmental as diagnosed. The head is unchanged at 4cfc9b02b.

@SEZ9 could you take the formal recheck of F1-F8 against this head? With CI green, the remaining gate is a final approval from a write-capable committer.

SEZ9 · 2026-10-31T02:06:37Z

Thanks @wgzhao — yes, I'll take the formal recheck of F1-F8 against 4cfc9b02b. Here's where it stands right now and what I still need before I can sign it off:

What I can confirm from your pointers

F1 / F3 (found: false docs): your rest-api-v1.md and rest-api-v2.md line pointers cover all four endpoints (v1 job-info, v1 running-job, v2 deprecated running-job, v2 job-info) plus the ZH mirrors, which is exactly the gap those two findings described. I'll verify the rendered JSON examples match what JobInfoService actually emits.
F2 / F4 (CLI polling): stopping only on an explicit found: false, tolerating one status-less / non-dict body, and printing a distinct response-shape / failover message on a repeat is the behaviour I was after.
F5 (wording): if the only remaining "id may be wrong" text is in run_diagnose's 404 branch where the id is user-supplied, that resolves the finding.
F6 / F8 (UI): loadError on initial-load rejection, warn-once-and-retry on post-load rejection, and { jobId } in detail.notFound.description with the 123456789 assertion all address what I raised.
F7 (job_exists): raising TypeError for non-dict / non-JSON before checking found is the right split.

Remaining asks (none depend on each other, so please provide all in one go)

For F2/F4, a short paste of the test case(s) in the CLI suite that exercise the one-tolerated-then-fail path and the "non-dict 200 followed by found: false" path, so I can confirm the retry counter is reset correctly after a good status response.
For F6, confirm whether the "warn once per error streak" counter in detail.tsx resets after a successful refresh — the two detail.spec.ts cases you named cover reject-on-load and reject-after-load, but I don't see one for reject → success → reject.
For F1/F3, a confirmation that the found: false example in the deprecated v2 /running-job/:jobId section carries the same deprecation note as the rest of that section, so readers aren't steered toward the old path by the new example.

CI being green on the rerun is noted and the Doris container timeout explanation matches what I'd expect for that suite. Once I have the three items above I'll post the formal recheck result for F1-F8 and hand off for committer approval.

DanielLeens · 2026-10-31T02:47:25Z

Thanks @wgzhao and @SEZ9. Build is now SUCCESS on the unchanged reviewed head, so my earlier Doris-startup retry request is superseded and my CI objection is cleared. I checked the three follow-up points in source:

CLI: test_polling_stops_when_the_job_is_not_known in test_diagnose_cli.py:330-358 asserts one call for found:false and two for repeated status-less responses; test_polling_recovers_from_one_status_less_response at :361-380 covers recovery to FINISHED. cli.py:1400 resets missing_response_seen on a valid status, and the explicit found:false branch runs before shape handling. I did not find a dedicated non-dict-then-found:false test or a status-less/good/status-less sequence in that fixture, so I am not claiming those exact sequences are tested.
UI: detail.tsx:121 resets lastRefreshError after a good status. src/tests/detail.spec.ts:344-403 explicitly covers repeated rejection, a successful refresh, then rejection again, and asserts warning counts 1, 1, 2, 2, 3. This test already exists on the reviewed head.
Docs: the deprecated v2 running-job section carries the deprecation note at rest-api-v2.md:741, and its found:false example at :813-823 links back to job-info. The example remains inside that deprecated section.

My existing source conclusion remains clear. These pointers should help the independent reviewer complete the requested recheck without treating already-present UI coverage as missing; @SEZ9 can separately resolve the specific additional CLI coverage request.

Checked 2026-10-31T02:47:20.919081+00:00; unchanged head 4cfc9b02bfee4a326e042b79e53b7bcfe1fad857. Signal pull request update=SUCCESS; Lint=SUCCESS; Test with Python 3.9=SUCCESS; Test with Python 3.13=SUCCESS; Test with Python 3.10=SUCCESS; Test with Python 3.13=SUCCESS; Build=SUCCESS.

This is a discussion/CI clarification, not another full source review or an effective maintainer approval. Daniel has READ repository permission; a write-capable maintainer owns required approval/dismissal.

wgzhao · 2026-10-31T03:09:53Z

Thanks @SEZ9 and @DanielLeens for the independent check. Here are the three items, all against the unchanged head 4cfc9b02b.

1. F2/F4 — CLI polling tests and the counter reset

seatunnel-cli/tests/test_diagnose_cli.py:331-358 parametrizes the two terminating paths:

@pytest.mark.parametrize(
    "info, expected_calls, expected_message",
    [
        ({"jobId": "852", "found": False}, 1, "no longer present"),
        ({"jobId": "852"}, 2, "returned no job status"),
    ],
    ids=["new-server-marker", "old-server-status-less"],
)
def test_polling_stops_when_the_job_is_not_known(...):
    ...
    assert calls["count"] == expected_calls
    assert expected_message in out

So found: false stops on the first poll, while two status-less responses are tolerated once and then terminate with the response-shape / failover message.

test_polling_recovers_from_one_status_less_response (:361-379) covers recovery:

responses = iter([{"jobId": "852"}, {"jobId": "852", "jobStatus": "FINISHED"}])
...
assert calls["count"] == 2
assert "Status: FINISHED" in out

The reset itself is cli.py:1398 (missing_response_seen = False), executed only after a valid jobStatus; the found: false check is at :1375, before the status-less handling at :1386-1396.

To be precise: there is no dedicated test for a non-dict 200 followed by found: false, and the recovery test does not send another status-less body afterwards. The branch order makes that first sequence deterministic (the explicit marker is checked before the shape handling), but if you consider those two exact sequences required, I can pin them in the existing test function.

2. F6 — warn-once counter reset in the UI

detail.tsx:122 resets lastRefreshError after every successful getJobInfo. detail.spec.ts:344-403 (should warn once per error streak and reset after a successful refresh) already covers reject -> success -> reject with this mock sequence:

.mockResolvedValueOnce(runningJob)                               // initial load
.mockRejectedValueOnce(new Error('blip'))                        // warning 1
.mockRejectedValueOnce(new Error('blip'))                        // same streak: no new warning
.mockRejectedValueOnce(new Error('other blip'))                  // warning 2
.mockResolvedValueOnce({ ...runningJob, jobName: 'Recovered' })  // resets
.mockRejectedValueOnce(new Error('blip'))                        // warning 3 after reset

and asserts warning counts 1, 1, 2, 2, 3 across the successive refresh ticks, so the reject-after-success case is covered on this head.

3. F1/F3 — deprecated v2 /running-job section

The deprecation note is the section header at docs/en/engines/zeta/rest-api-v2.md:741 ("This API has been deprecated, please use /job-info/:jobId instead"), immediately above the endpoint's <details> at :743-825. The found: false example at :813-820 is inside that block and links back to /job-info at :822-823, so it does not steer readers toward the old path.

CI is green on the unchanged head and the Doris container timeout did not recur on the rerun. @SEZ9 once you have confirmed the above, the formal recheck and a write-capable approval are the remaining gates.

DanielLeens

Thanks @wgzhao for the thorough pointers, and for rerunning the Doris job. I re-checked your answers against the unchanged head 4cfc9b02bfee4a326e042b79e53b7bcfe1fad857, and everything holds, so I'm switching this to an approval.

CI. The rerun cleared the earlier failure: apache-side Build (run 123923768320) is pass, along with Lint, Signal pull request update and the Python 3.9/3.10/3.13 jobs. The earlier red was the Doris container-startup timeout in DorisSchemaChangeIT, which does not touch /job-info. No CI blocker remains.

Verification of the claims on this head

F2/F4 (CLI polling), cli.py _poll_job_status: the explicit found is False check runs first and returns; a status-less or non-dict body is tolerated once (missing_response_seen), a repeat prints the response-shape/failover message, and the flag is reset only after a valid status. This matches your description. I agree with your own caveat: there is no dedicated test for non-dict-then-found:false or status-less/good/status-less. The branch order makes the first deterministic, so I treat this as an optional hardening, not a blocker.
F6/F8 (UI), detail.tsx: res.found === false and a repeated status-less body set missing; a rejection before first load sets loadError, a rejection after load keeps the page, warns once per distinct error and retries; lastRefreshError is reset after a successful refresh. detail.spec.ts has the reject, success, reject case (should warn once per error streak and reset after a successful refresh). One small note: the dedupe key is the error message, so two different errors in one streak warn twice -- this is what the test intends and is fine.
F7 (SDK) JobsApi.job_exists: non-dict raises TypeError before found is read; fallback to "jobStatus" in details for older engines.
Server: JobInfoService adds RestConstant.FOUND=false only on the final unknown-id branch; running and finished payloads are unchanged.
Docs: the deprecated v2 /running-job section keeps its deprecation note (rest-api-v2.md:741) and the found:false example sits inside it and links back to /job-info. EN/ZH are both updated.

Compatibility / risk. Fully compatible: HTTP 200 and all existing fields are unchanged; only the previously bare {jobId} body gains one key, and new clients fall back to "no jobStatus" for older engines. Tests are deterministic (stubbed sleep, fake timers restored in afterEach).

Issue summary: no open source-level issues. Open items from @SEZ9's recheck are answered with evidence above.

Conclusion: Ready to merge

Optional non-blocking follow-up: pin the non-dict-then-found:false sequence in test_polling_stops_when_the_job_is_not_known if @SEZ9 wants it. A write-capable maintainer approval is still required to merge. Thanks for the careful and consistent work across the server, UI, SDK and CLI!

davidzollo

+1
LGTM 🚀

Huge thanks for your contribution!

github-actions Bot added document Zeta Zeta Rest API labels Oct 19, 2026

SEZ9 reviewed Oct 20, 2026

View reviewed changes

DanielLeens reviewed Oct 20, 2026

View reviewed changes

wgzhao force-pushed the fix/rest-job-info-found-12785 branch from 54944fa to ed6432b Compare October 20, 2026 04:03

wgzhao force-pushed the fix/rest-job-info-found-12785 branch from ed6432b to 3908e3b Compare October 20, 2026 05:04

DanielLeens reviewed Oct 20, 2026

View reviewed changes

wgzhao force-pushed the fix/rest-job-info-found-12785 branch from 3908e3b to 1747a27 Compare October 20, 2026 11:36

DanielLeens reviewed Oct 21, 2026

View reviewed changes

wgzhao force-pushed the fix/rest-job-info-found-12785 branch from 1747a27 to 489dd37 Compare October 21, 2026 00:37

wgzhao force-pushed the fix/rest-job-info-found-12785 branch from 489dd37 to 86e7508 Compare October 21, 2026 02:58

DanielLeens reviewed Oct 21, 2026

View reviewed changes

wgzhao force-pushed the fix/rest-job-info-found-12785 branch from 86e7508 to 21780a7 Compare October 21, 2026 07:06

DanielLeens reviewed Oct 21, 2026

View reviewed changes

DanielLeens reviewed Oct 22, 2026

View reviewed changes

wgzhao force-pushed the fix/rest-job-info-found-12785 branch from 3578e25 to 875a7ff Compare October 23, 2026 07:40

DanielLeens reviewed Oct 23, 2026

View reviewed changes

wgzhao added 2 commits October 28, 2026 17:55

[Improve][Zeta] Add found marker to unknown job-info responses (apach…

6847d86

…e#12785)

[Test][Zeta] Reset fake timers after each UI detail test

4cfc9b0

wgzhao force-pushed the fix/rest-job-info-found-12785 branch from 875a7ff to 4cfc9b0 Compare October 28, 2026 09:56

DanielLeens reviewed Oct 28, 2026

View reviewed changes

DanielLeens approved these changes Oct 31, 2026

View reviewed changes

github-actions Bot added the reviewed label Oct 31, 2026

davidzollo approved these changes Oct 31, 2026

View reviewed changes

github-actions Bot added the approved label Oct 31, 2026

Conversation

wgzhao commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of this change

Changes

Compatibility

Release note

Review follow-up

Tests

CI note

Uh oh!

SEZ9 left a comment

Choose a reason for hiding this comment

What this PR fixes

Runtime chain I rechecked

Findings

Review conclusion

Conclusion: final approval remains pending on the previous comments

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What Problem Does This PR Solve?

1. Code Change Review

1.1 Core Logic Analysis

1.2 Compatibility Impact

1.3 Performance / Side-Effect Analysis

1.4 Error Handling and Logging

2. Code Quality Assessment

2.1 Coding Standards

2.2 Test Coverage and Test Stability

2.3 Documentation Updates

3. Architectural Soundness

3.1 Elegance of the Solution

3.2 Maintainability

3.3 Extensibility

3.4 Historical-Version Compatibility

4. Issue Summary

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Uh oh!

wgzhao commented Oct 20, 2026

Uh oh!

wgzhao commented Oct 20, 2026

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What Problem Does This PR Solve?

1. Code Change Review

1.1 Core Logic Analysis

1.2 Compatibility Impact

1.3 Performance / Side-Effect Analysis

1.4 Error Handling and Logging

2. Code Quality Assessment

2.1 Coding Standards

2.2 Test Coverage and Test Stability

2.3 Documentation Updates

3. Architectural Soundness

3.1 Elegance of the Solution

3.2 Maintainability

3.3 Extensibility

3.4 Historical-Version Compatibility

4. Issue Summary

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Uh oh!

wgzhao commented Oct 20, 2026

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What Problem Does This PR Solve?

1. Code Change Review

1.1 Core Logic Analysis

1.2 Compatibility Impact

1.3 Performance / Side-Effect Analysis

1.4 Error Handling and Logging

2. Code Quality

2.1 Coding Standards

2.2 Test Coverage and Test Stability

2.3 Documentation

3. Architectural Soundness

wgzhao commented Oct 19, 2026 •

edited

Loading