fix: stopProject 中のジョブキャンセル誤判定を修正 (aicshud/WHEEL#1016 を分割: #1017, #1018, #1019) - #139
Merged
so5 merged 10 commits intoSep 16, 2026
Merged
Conversation
Documents the standard process for bug fixes going forward: file a GitLab issue for the symptom, comment the investigation, write a reproduction test and confirm it's red before committing, implement the fix and confirm green before committing, push to the fork and confirm CI is green, then open the PR upstream. Lint runs before every commit in the sequence, and when one PR bundles multiple issues each issue's investigate/red-test/fix cycle is done sequentially rather than in parallel. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3jKNM1qubomM8UTRdkEWu
Adds failing tests demonstrating that a cancelled job (PC=1, e.g. via pjdel/stopProject) is misjudged as not-failed: - isJobFailed(): with a real jobScheduler.json shape (numeric acceptableJobStatus, e.g. Fugaku's [0, 6]), PC=1 should be judged failed but currently isn't, due to the function's reversed polarity plus a numeric/string type mismatch against Array.prototype.includes. - getStatusCode(): when the script's own return code (EC) is not obtainable (strRt === null), it should fall back to the job status code (PC) via isJobFailed() per the EC/PC design policy, instead of the current hardcoded -2 that ignores task.jobStatus entirely. Confirmed red: 3 failing (server/scratchpad/jobmanager-repro-test-output.log). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3jKNM1qubomM8UTRdkEWu
isJobFailed(): acceptableJobStatus is the list of codes that mean "OK", so a code is failed when it is NOT in that list - the implementation had this inverted, and additionally compared the (always numeric, per jobScheduler.json) acceptableJobStatus entries against a (always string, regexp capture) code without normalizing types, so the buggy comparison always fell through to false regardless of polarity. Existing unit tests were written to match the old (wrong) polarity; their expectations are corrected accordingly. getStatusCode(): when the script's own return code (EC) is not obtainable/trustworthy (strRt === null), fall back to the job status code (PC) via isJobFailed(JS, task.jobStatus) instead of a hardcoded -2 that ignored task.jobStatus and could fail jobs whose PC was actually fine. Removes the strRt === "6" special case, which tried to detect "canceled by a stepjob dependency expression" (PC=6) by checking whether the *return code* happened to read "6" - that's a PC concept, not an EC one, and duplicated/conflicted with acceptableJobStatus's own PC=6 entry; the new PC fallback covers it via the correct code path. jobScheduler.json (Fugaku): reReturnCode no longer matches CCL (canceled) rows, only EXT (normal exit) ones - a canceled job's EC column is meaningless, so it must go through the null-fallback above rather than being read as if it were the script's real exit code. Full server test suite: 1141 passing, including all #isJobFailed, #getStatusCode and #registerJob cases. 37 unrelated pre-existing failures (`git: 'lfs' is not a git command` in gitOperator2.js's before-each hooks) come from this host missing the git-lfs binary and are unaffected by this change. Test output: server/scratchpad/jobmanager-repro-test-output.log Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3jKNM1qubomM8UTRdkEWu
Adds failing tests demonstrating that stopProject's job cancellation
never stops jobManager.js's background job-status polling for that
job:
- registerJob() should stash a jobManagerRequestId/jobManagerCancel
closure on the task so callers can later stop watching it; calling
jobManagerCancel() should delRequest and resolve(null) (mirroring a
killed local task's resolved, not rejected, exit so the SBS
wrapper's "not-started" guard discards it).
- cancelRemoteJob() should call cancelJobStatusCheck(task) once pjdel
actually succeeds, so the poller is only stopped when the job is
confirmed cancelled (not on a failed pjdel, where the job might
still be running and WHEEL should keep tracking it).
Adds a `cancelJobStatusCheck: ()=>{}` placeholder to taskUtil.js's
_internal purely as a test seam (sinon needs an existing function to
stub); it is not yet imported from jobManager.js or called from
cancelRemoteJob() - that wiring is the fix commit.
Confirmed red: 3 failing (server/scratchpad/jobmanager-repro-test-output.log).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C3jKNM1qubomM8UTRdkEWu
…icshud/WHEEL#1018)
registerJob() now stashes a jobManagerCancel closure (and the
underlying rwatchd request id) on the task. Calling it delRequest()s
the poll and resolve(null)s the registerJob() promise - resolve, not
reject, so the SBS wrapper's "task.state === 'not-started'" guard
(executerManager.js) discards it exactly like a killed local task's
resolved (not rejected) exit. All natural settle paths ("checked" ->
max status check error, "finished", "failed") clear the stashed
closure/id so a later cancel is a no-op.
Exports cancelJobStatusCheck(task) as the public entry point.
taskUtil.js's cancelRemoteJob() calls it only *after* ssh.exec(pjdel/
scancel/...) has actually succeeded - if the cancel command itself
fails (e.g. a connection error), the job might still be running, so
polling is deliberately left running rather than losing track of it.
Full server test suite: 1147 passing, including all #registerJob and
#cancelRemoteJob cases. Same 37 pre-existing unrelated failures as
before this branch (git-lfs binary missing on this host, unrelated to
this change). Test output: server/scratchpad/jobmanager-repro-test-output.log
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C3jKNM1qubomM8UTRdkEWu
Adds failing tests demonstrating that a stale component/task-state update arriving after the project has already been torn down (eventEmitters.delete(projectRootDir) already ran) crashes with TypeError: Cannot read properties of undefined (reading 'emit'): - execUtils.js's setTaskState() - dispatcher.js's Dispatcher#_setComponentState() Both call eventEmitters.get(projectRootDir).emit(...) without checking the map lookup for undefined first. Confirmed red: 2 failing, at the exact lines from the original crash (server/scratchpad/jobmanager-repro-test-output.log). Note: got these two tests running at all required installing git-lfs on this host (server/app/core/gitOperator2.js's gitInit unconditionally runs `git lfs install`, so any test using createNewProject - most of dispatcher.js's suite included - was already failing on this host before this branch; installed the git-lfs v3.5.1 static binary to ~/.local/bin, no repo changes). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3jKNM1qubomM8UTRdkEWu
docker compose's --wait reports the test PBS container "healthy"
slightly before pbs_server is actually ready to accept qmgr
connections, so the one-shot `qmgr -c "set server
job_history_enable=True"` could silently fail ("qmgr: cannot connect
to server", exit code ignored by the script). With job_history_enable
never actually set, a finished job's qstat/qstat -xf record can be
purged from PBS's queue before WHEEL's status polling reads it, making
the status-check command itself fail (nonzero exit, not in
acceptableRt) - previously this was masked by the isJobFailed bug this
branch fixes (aicshud/WHEEL#1017), so it silently resolved as success;
now it correctly surfaces as a rejection, which is what exposed the
race in the first place.
Retries the qmgr call for up to ~20s. Verified: 3 previously-failing
"#remote job" tests in server/test/app/core/executer.js (which were
failing with "the number -2 was thrown" - i.e. a real environment bug,
not a logic bug in this branch's fix) now pass consistently.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C3jKNM1qubomM8UTRdkEWu
…aicshud/WHEEL#1019)
execUtils.js's setTaskState() and dispatcher.js's
Dispatcher#_setComponentState() both look up this project's
eventEmitters entry and call .emit() on it unconditionally. If the
task/component settles after the project has already been torn down
(eventEmitters.delete(projectRootDir) already ran - e.g. a job-status
poll that outlived stopProject, or any other late-resolving
completion), the lookup returns undefined and .emit() throws
TypeError: Cannot read properties of undefined (reading 'emit').
There is no one left to notify in that case, so both now just skip
the emit when the map has no entry for the project, instead of
crashing.
Full server test suite (server/scratchpad/jobmanager-repro-test-output.log):
1664 passing, both new #1019 reproduction tests included. 3 remaining
failures ("shared storage between localhost and remote host" in
dispatcher.js) are unrelated to this change - task1 in those tests is
a plain remote task (no job scheduler involved at all), and neither
this commit nor #1017/#1018 touch that code path; traced to a
pre-existing shared-storage mount/permission issue in this local test
environment.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C3jKNM1qubomM8UTRdkEWu
This reverts commit 7b66d33.
…est run
npm run test -w server runs mocha natively on the host, with only the
PBS testbed containerized - the skill's own description ("we need to
use docker container... to make sure the test environment is
consistent and isolated") was actually describing npm run testDocker
-w server, which builds and runs the entire suite (mocha included)
inside its own container via server/test/compose.yml's
wheel_release_test service. The host-native variant must never be
invoked directly.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C3jKNM1qubomM8UTRdkEWu
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要
stopProject 実行時、投入済みのジョブスケジューラタスク（例: Fugaku へ
pjsubされたジョブ）をキャンセルすると、project がstoppedにならずfinishedになってしまう問題（aicshud/WHEEL#1016）の調査・修正です。調査の結果、原因は独立した3つの問題に切り分けられたため、
#1016は3つのissueに分割し（本PRで解消後、#1016自体はclose済み）、それぞれについて再現テスト（red）→修正（green）のコミットを積んでいます。aicshud/WHEEL#1017: ジョブのキャンセル（PC=1）が正常終了と誤判定される（isJobFailed()の極性・型バグ、reReturnCodeのCCL/EXT混同、getStatusCode()のPCフォールバック欠如）aicshud/WHEEL#1018: stopProject でジョブをキャンセルしてもジョブステータスのバックグラウンドポーリングが止まらないaicshud/WHEEL#1019: project 破棄後に遅延したタスク完了イベントが届くとeventEmittersがundefinedで crash する（汎用的な防御漏れ）変更内容
server/app/db/jobScheduler.json（Fugaku）:reReturnCodeからCCL（キャンセル）を除外し、EXT（正常終了）のみを対象にするserver/app/core/jobManager.js:getStatusCode(): スクリプトの戻り値（EC）が取得できない場合、固定値-2ではなくisJobFailed(JS, task.jobStatus)によるジョブステータスコード（PC）ベースの判定にフォールバックする（EC が信頼できる時はEC、できない時はPCを使うという設計ポリシーに準拠）isJobFailed():acceptableJobStatus（「これが来たらOK」というコードのリスト）に対する判定の極性を反転し、数値/文字列の型不一致も解消registerJob(): バックグラウンドのジョブステータスポーリングを明示的に停止できるcancelJobStatusCheck()を新規エクスポートserver/app/core/taskUtil.js:cancelRemoteJob()から、pjdel等のキャンセルコマンドが成功した後にのみcancelJobStatusCheck()を呼ぶ（キャンセル自体が失敗した場合はポーリングを継続し、ジョブの行方を見失わないようにする）server/app/core/execUtils.js/server/app/core/dispatcher.js: project 破棄後に遅延した completion イベントが届いてもeventEmittersの存在チェックを行い、TypeErrorで crash しないようにするAGENTS.md: 今回の対応を踏まえ、今後のバグ修正作業の標準手順（test-first: issue起票→調査コメント→再現テスト(red)→修正(green)→フォークでCI確認→PR）を明文化skills/server-side-test/SKILL.md: サーバサイドのユニットテストはnpm run testDocker -w server（mocha含め全体をコンテナ内で実行）を使うべきところ、ホスト上でmochaを直接実行するnpm run test -w serverを案内していたため修正テスト
npm run testDocker -w server（mochaを含め全体をコンテナ内で実行する、正規の実行方法）で確認済みです。1652 passing / 0 failing / 15 pending。maintenance2026 / maintenance2023 について
調査の結果:
maintenance2026は本質的に main と同一のバグを持っています（jobManager.js/jobScheduler.jsonの該当箇所が完全に同一）。backport は別途対応します。maintenance2023はisJobFailed/acceptableJobStatusという抽象自体が存在しない古い設計で、同系統の根本原因（EXT/CCLの混同）は独立に持ちますが、パッチの当て方が全く異なります。こちらも別途対応します。maintenance2023という名前がブランチとタグの両方に存在し別コミットを指していた問題（git show maintenance2023:...等が意図せずタグ側を指してしまう）は、タグ側（誤って付与されていたもの、紐付いていたGitHub Releaseもろとも）を削除して解消済みです。関連（別途対応、本PRのスコープ外）
aicshud/WHEEL#106（stop/pauseボタン押下時にリモートホストで実行中のプロセスが殺せない）に、今回の調査で分かった追加情報（remoteExecのkillTask()が何もしないことの副作用）をコメント済み🤖 Generated with Claude Code
https://claude.ai/code/session_01C3jKNM1qubomM8UTRdkEWu