Skip to content

Job heartbeat, batched record file counts, blank number values, dead code cleanup - #4593

Merged
SteRiccio merged 1 commit into
masterfrom
job-heartbeat-records-counts-cleanup
Oct 29, 2026
Merged

SteRiccio merged 1 commit into
masterfrom
job-heartbeat-records-counts-cleanup

Conversation

@SteRiccio

Copy link
Copy Markdown
Member

Description

Follow-up to #4591 and #4592 (group 4 of the code audit).

  • Job heartbeat (server/job/jobThreadExecutor.js, server/job/jobRepository.js)

    The stale jobs cleanup marks a job as failed when its row hasn't changed for 60 minutes, but a job updates its row only when it reports progress: a long step without progress (e.g. an RDB refresh on a big survey) looked dead, and once marked failed another job could start on the same survey. Every 5 minutes each process now touches date_modified of the jobs it is running (only rows still pending/running, so a job already marked failed/canceled is not revived).
  • Records summary counts (recordManager.js, fileRepository.js, nodeRepository.js)

    With includeCounts, files count/size and missing files were fetched with 2 queries per record (≈100k queries for a 50k records export). Now one grouped query per counter for all the records, merged in memory.
  • Blank number values (core/numberUtils.ts, validatorFunctions.ts)

    NumberUtils.toNumber(' ') returned 0 (so isInteger(' ') was true, e.g. in Collect import date/time parts and in the RDB integer/decimal columns), and Validator.validateNumber accepted ' '. Blank strings are now not numbers. Note: Numbers in @openforis/arena-core has the same behaviour, so it was not used here.
  • Dead code removed

    ENV.dbUrl/pg* settings and the DATABASE_URL parsing in core/processUtils.ts (DB config comes from arena-server), validatePositiveNumber (and the allowZero param only it used), DateUtils.addHours/addMinutes/formatDateDisplay, Expression.isCall/isSequence.

Types of changes

  • Docs change
  • Refactoring
  • Dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

How has this been tested

  • New unit tests in test/unit/tests/063numberUtilsBlankValues.test.ts (blank values in NumberUtils and validateNumber).
  • The new SQL (grouped files count/size, grouped missing files, jobs touch) was run against a local PostgreSQL 16 with sample rows: per-record counts and sizes as expected (deleted files excluded), only running/pending jobs touched.
  • yarn test:unit: 1342 passed, 4 failed (the same 4 job tests that fail on master).
  • ESLint clean on the changed files; yarn typecheck reports no new errors.

Do you consider this PR needs further testing?

  • No
  • Yes

Worth checking the records list with counts (files count / size / missing files columns) on a survey with file attributes.

UI changes

None.

Disclaimer

Queued jobs that are still waiting in a process's queue (no worker thread yet) are not touched by the heartbeat.


Generated by Claude Code

…de cleanup

- jobThreadExecutor: touch the rows of the jobs running on this process every 5 minutes,
  so a long step without progress updates is not marked as failed by the stale jobs cleanup
- records summary with counts: fetch files count/size and missing files with one grouped
  query per counter instead of two queries per record
- NumberUtils.toNumber and Validator.validateNumber: blank strings are not numbers
- remove unused code: ENV DB settings (DB config comes from arena-server),
  validatePositiveNumber, DateUtils.addHours/addMinutes/formatDateDisplay,
  Expression.isCall/isSequence

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015iBJ7y2dfG18xPFFCtmYEz
@sonarqubecloud

Copy link
Copy Markdown

@mergify

mergify Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

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.

@SteRiccio
SteRiccio requested a balanced review from Copilot October 29, 2026 12:24
@SteRiccio SteRiccio self-assigned this Oct 29, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The concurrent per-process heartbeat touching active rows and a broad numeric-parsing change carry significant risk while the heartbeat lacks automated tests and the records-with-counts path still needs manual verification, exceeding what can be confidently assessed here.

No open issues 🎉

What changed in this PR

This PR is group 4 of a code audit (follow-up to #4591/#4592). It bundles four unrelated maintenance changes: a reliability fix for the job system, a performance fix for record summary counts, a correctness fix for blank numeric values, and removal of dead code.

Changes:

  • Job heartbeat: each process now touches date_modified of its still-pending/running jobs every 5 minutes (jobThreadExecutor + new jobRepository.touchActiveJobs), so a long step without progress is no longer falsely reaped by the 60-minute stale-jobs cleanup; the status filter prevents reviving already-failed jobs.
  • Batched record file counts: fetchRecordsSummaryBySurveyId with includeCounts replaces ~2 queries per record with one grouped query per counter (new fetchCountAndTotalFilesSizeByRecordUuids and countNodesWithMissingFileByRecordUuids), merged in memory.
  • Blank numeric values: NumberUtils.toNumber(' ') now returns NaN instead of 0, and Validator.validateNumber rejects blank strings; plus removal of dead code (ENV DB settings, validatePositiveNumber, DateUtils.addHours/addMinutes/formatDateDisplay, Expression.isCall/isSequence).
File Description
server/​job/​jobThreadExecutor.js Adds a 5-min setInterval heartbeat (unref'd) that touches active job rows.
server/​job/​jobRepository.js New touchActiveJobs updating date_modified for pending/running jobs.
server/​system/​schedulers/​staleJobsCleanup.js Comment updated to reflect the new heartbeat rationale.
server/​modules/​record/​repository/​nodeRepository.js New batched countNodesWithMissingFileByRecordUuids.
server/​modules/​record/​repository/​fileRepository.js New batched fetchCountAndTotalFilesSizeByRecordUuids.
server/​modules/​record/​manager/​recordManager.js Uses the two batched queries instead of per-record loops.
core/​numberUtils.ts toNumber treats blank strings as non-numbers (NaN).
core/​validation/​_validator/​validatorFunctions.ts validateNumber uses toNumber; removes validatePositiveNumber and allowZero.
core/​validation/​validator.ts Drops the validatePositiveNumber re-export.
core/​processUtils.ts Removes dead DATABASE_URL/pg* ENV parsing (DB config comes from arena-server).
core/​dateUtils.ts Removes unused addHours/addMinutes/formatDateDisplay.
core/​expressionParser/​expression.ts Removes unused isCall/isSequence.
test/​unit/​tests/​063numberUtilsBlankValues.test.ts New unit tests for blank-value handling in NumberUtils and validateNumber.

No blocking issues were found. Note that the records export path (includeCounts without a limit) can pass a very large record-UUID list into a single IN (...) clause; this is a large improvement over the previous ~100k round-trips, but is worth confirming against the biggest surveys as the author suggested.

Review effort: Balanced

@SteRiccio
SteRiccio merged commit d1e6dd8 into master Oct 29, 2026
15 checks passed
@SteRiccio
SteRiccio deleted the job-heartbeat-records-counts-cleanup branch October 29, 2026 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants