Several bugs in the job manager (lib/job.js): daemon crash on missing log, assignment-vs-comparison in queue scan, fd/worker leaks
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 条评论