Repository navigation
Fix job thread cleanup, cross-process job cancel, cycles validation and SQL filter conversion - #4592
Conversation
…SQL filter conversion - jobThreadExecutor: track job threads by job uuid; the (throttled) end notification of a job no longer terminates/removes the thread or the active summary of a newer job of the same user - jobManager: relay job cancel requests to the other server processes via ClusterBus - surveyCyclesValidator: apply the cycles count errors to the validation result - Expression SQL mode: convert AND/OR/= only outside quoted values and as whole words Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015iBJ7y2dfG18xPFFCtmYEz
|
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
There was a problem hiding this comment.
🔵 Needs a closer look
The concurrency-sensitive worker-thread lifecycle and cross-process cancellation changes lack automated test coverage and runtime validation, leaving their correctness unverified.
No open issues 🎉
What changed in this PR
This PR is group 3 of a code audit follow-up to #4591. It fixes four independent, self-contained bugs around background-job lifecycle/cancellation, survey cycles validation, and SQL-mode filter conversion, plus a new unit test file covering the testable parts.
Changes:
- Job thread cleanup (
jobThreadExecutor.js): threads are now also tracked by job uuid, so a throttled end-notification for a finished job terminates only its own thread (and updates the user's thread map/active summary only when it is still the user's current job), instead of killing the next queued job's thread. - Cross-process job cancel (
jobManager.js): cancel-by-user and cancel-by-uuid requests are relayed overClusterBusso other server processes cancel a job they actually run; the publishing process ignores its own echoed event via anoriginProcessIdguard. - Cycles validation + SQL filter conversion: the curried
Validation.setErrors/setValidare now actually applied to the result (0 / >10 cycles now fail validation); SQL↔JS operator conversion is applied only outside quoted literals, withAND/ORmatched as whole words.
Verification notes: I confirmed the Validations setters are side-effecting (consistent with the existing loop and sibling validators like nodeDefExpressionsValidator.js), that the ClusterBus publish/onEvent/_handleClusterEventForTest pattern matches the established usage in surveyRecordsThreadService.js, that queue.cancelJobByUserUuid/cancelJobByUuid, uuidv4, and the validation message keys all exist, and that the expression operator round-trips hold for the quoted-value and whole-word cases the tests assert. The author has explicitly flagged that the worker-thread fix has no automated test and the multi-process cancel path was not runtime-validated.
| File | Description |
|---|---|
server/job/jobThreadExecutor.js |
Adds per-job-uuid thread tracking so a finished job only cleans up its own thread/summary. |
server/job/jobManager.js |
Relays job-cancel requests across processes via ClusterBus, ignoring the publisher's own event. |
core/survey/_surveyValidator/surveyCyclesValidator.js |
Applies the curried setErrors/setValid to the result so cycles-count validation actually fails. |
core/expressionParser/expression.ts |
Converts SQL↔JS operators only outside quoted literals, matching AND/OR as whole words. |
test/unit/tests/062securityAuditFixes.test.ts |
New unit tests for cycles validation, quoted-value SQL round trips, and cross-process cancel publishing. |
Review effort: Balanced



Description
Follow-up to #4591 (group 3 of the code audit).
Job thread cleanup (
Worker threads were tracked only by user. A job's final update goes through a 500 ms throttle, whileserver/job/jobThreadExecutor.js)JobQueuestarts the user's next queued job right away: the delayed end notification then found the new job's thread, terminated it 1 s later and removed it from the map (the new job could no longer be canceled), and cleared its active summary. Threads are now also tracked by job uuid: a finished job only terminates its own thread, and touches the user's thread map / active summary only if it is still the user's current job.Job cancel across server processes (
server/job/jobManager.js)cancelJobByUserUuid/cancelJobByUuidonly looked at the local process, so with several processes a cancel request could land on a process that doesn't run the job and silently do nothing. Cancel requests are now also published onClusterBus; the other processes cancel the job if they run it, and the publishing process ignores its own event.Survey cycles count validation (
The curriedcore/survey/_surveyValidator/surveyCyclesValidator.js)Validation.setErrors/setValidwere never applied to the result, so 0 cycles or more than the maximum (10) passed validation. Existing surveys with more than 10 cycles will now show a validation error when editing the survey info.SQL-mode filter conversion (
core/expressionParser/expression.ts)AND/OR/=were replaced everywhere, including inside quoted values (species = 'ORANGE'became'||ANGE'). They are now converted only outside quoted values, andAND/ORonly as whole words; same for the reverse conversion used to display the filter.Types of changes
How has this been tested
test/unit/tests/062securityAuditFixes.test.ts: cycles count validation, SQL-mode filter round trip with quoted values, cancel requests published onClusterBus.yarn test:unit: 1331 passed, 4 failed (the same 4 job tests that fail onmasterwithout these changes).yarn typecheckreports no new errors.Do you consider this PR needs further testing?
The job thread fix has no unit test (it needs real worker threads), and the cross-process cancel was not tested with several server processes: worth a manual check by running two jobs in a row for the same user, and canceling a job from a different process.
UI changes
None.
Disclaimer
Not run against a live server.
Generated by Claude Code