ITADN

Several bugs in the job manager (lib/job.js): daemon crash on missing log, assignment-vs-comparison in queue scan, fd/worker leaks

#982Openfl4p 创建于 2026-06-14
F
fl4pcommented
While investigating repeated worker crashes I found several distinct bugs in `lib/job.js` (in addition to the EBADF fd crash in #981). All are verified against the code; none change happy-path behavior. A branch with fixes is linked at the bottom. ### 1. `finishLocalJob` — daemon crash when the job log file is missing (high) `stats` is left `null` when `fs.statSync` throws (the throw is caught and logged), but the assignment runs **outside** the try: ```js var stats = null; try { stats = fs.statSync( job.log_file ); } catch (err) { self.logError(...); } job.log_file_size = stats.size; // TypeError if stats === null ``` If the log file is gone at finish time (e.g. the process tree was OOM-killed/cleaned, or the log was removed), this throws an uncaught `TypeError` → `uncaughtException` → emergency shutdown. Same crash class as #981. Fix: `job.log_file_size = stats ? stats.size : 0;` ### 2. Assignment instead of comparison in the pending-job queue scan — 3 places (high) `updateLocalJob`, `abortLocalPendingJob`, and `watchJobLog` all scan `internalQueue` with: ```js if ((task.action = 'launchLocalJob') && (task.id == stub.id)) { ``` The `=` is an assignment — it overwrites every scanned task's `action` and is always truthy. (`getAllActiveJobs` uses the correct `==`.) This corrupts the internal queue: tasks of other action types get rewritten to `launchLocalJob`, which `getAllActiveJobs` then counts as pending jobs — inflating the active count and bypassing the `max_children` concurrency gate. Fix: `==` in all three. ### 3. `finishLocalJob` retry path — `this.kids` leak (medium) ```js delete job.pid; ... delete this.kids[ job.pid ]; // job.pid already deleted -> delete this.kids[undefined] (no-op) ``` The worker entry for the old PID is never removed; it leaks once per retry, and a later reused PID can be misattributed CPU/mem by `monitorServerResources`. Fix: capture the pid before deleting it. ### 4. `monitorServerResources` process-tree walk — mutates the object it iterates (medium) The family BFS adds and deletes keys of `family` while iterating it with `for (var fpid in family)`. Adding/removing properties during `for...in` is engine-defined; it risks missed descendants or double-counting (which feeds the memory/CPU sustain-limit aborts). Rewriting as an explicit frontier queue with a `seen` set is deterministic and also guards PID-reuse cycles. ### 5. `monitorServerResources` — callback can fire twice (medium) A failed `cp.exec` can invoke both the completion callback (with `err`) and the `child.on('error')` handler; both call `callback()`, so `monitorAllActiveJobs` runs twice on the same tick and can issue duplicate abort commands. Route all exits through a one-shot guard. ### 6. `watchJobLog` — fd leak on error (medium) If an `async.series` step fails after `fs.open` succeeded, the final error handler returns without closing `log_fd`, leaking one fd per failed watch attempt. ### 7. `chooseServer` — wrong debug log (low) `"...algo: " + event.algo || 'random'` — `+` binds tighter than `||`, so it logs the raw (possibly `undefined`) value. Wrap in parens. (Note: I also checked the `clearTimeout` vs `setInterval` handle in `watchJobLog` — that is **not** a bug; in Node `clearTimeout` cancels an interval handle fine.) Branch with all fixes (PRs appear restricted on this repo, so filing as an issue): https://github.com/fl4p/Cronicle/tree/fix/job-manager-bugs Diff: https://github.com/jhuckaby/Cronicle/compare/master...fl4p:Cronicle:fix/job-manager-bugs Related: #981 (EBADF fd double-close crash).
0 条评论