core: Energy monitoring - extend destroyStatesFrom (device) and wrapper (job) for partial recalc support - PR1

[Terdious]

Pull Request check-list

To ensure your Pull Request can be accepted as fast as possible, make sure to review and check all of these items:

• If your changes affect the code, did you write the tests?
• Are tests passing? (npm test on both front/server)
• Is the linter passing? (npm run eslint on both front/server)
• Did you run prettier? (npm run prettier on both front/server)
• Did you test this pull request in real life? With real devices? If this development is a big feature or a new service, we recommend that you provide a Docker image to the community (forum) for testing before merging.
• If your changes modify the API (REST or Node.js), did you modify the API documentation? (Documentation is based on comments in code)

NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.

Description of change

Summary

This PR is the first independent slice extracted from #2413 as discussed
on https://community.gladysassistant.com/t/amelioration-re-calcul-suivi-de-lenergie/10069 ;
the rest of #2413 will follow in subsequent PRs only if this one is merged.

Extends two existing core primitives with an optional parameter, in preparation for upcoming energy-monitoring features (selective recalculation, date-range recalculation). The change is intentionally incremental rather than introducing sibling functions: every pre-existing caller keeps the same effective behavior, no new file is added, no new public surface area is exposed.

• device.destroyStatesFrom(selector, from, to = new Date()) — adds an optional to upper bound (inclusive) defaulting to now.
• job.wrapper(type, func, { detached = false } = {}) — adds an optional { detached } option flag. When false (default), the wrapper body is byte-for-byte identical to master; when true, the wrapper starts the job, runs func in the background, returns the started job immediately and never re-throws to the caller.

Details

device.destroyStatesFrom(selector, from, to = new Date())

• The existing function previously had the signature (selector, from).
• The new optional to parameter defaults to new Date(), so every pre-existing caller keeps the same effective behavior: the SQL goes from WHERE created_at >= from to WHERE created_at >= from AND created_at <= now, which is identical in practice.
• When to is explicitly provided, the function deletes only states inside [from, to] (both inclusive).
• When from > to, the SQL is naturally empty and the function silently no-ops — documented in the JSDoc and covered by an explicit test.
• Continues to throw NotFoundError('DeviceFeature not found') for unknown selectors. Continues to use parameterized DuckDB DELETE (no string interpolation in SQL).

job.wrapper(type, func, { detached = false } = {})

• The existing function previously had the signature (type, func).
• When detached === false (default), the wrapper performs the exact same SQL sequence as on master (start → await func → finish(SUCCESS) / on error finish(FAILED) then re-throw) and returns the same value (res). The shared body is now extracted into a local IIFE (runAndFinish) so the legacy and detached modes share it instead of duplicating it.
• When detached === true, the wrapper calls this.start(type), fires runAndFinish() without awaiting it, and returns the started job immediately. Errors thrown by func are reported on the job (status=FAILED) and never re-thrown to the caller; a secondary try/catch falls back to logger.error('job.wrapper: failed to finish job ...') if finish(FAILED) itself rejects.
• This is the fire-and-forget mode needed by long-running "from beginning" recalculations that would otherwise hold the HTTP request open until completion.

Tests

• server/test/lib/device/device.destroyStatesFrom.test.js:
  • the 5 pre-existing master tests are unchanged,
  • 2 new tests cover the bounded form: a happy path with a range cut-out (states inside [from, to] deleted, states outside kept) and an inverted-range silent no-op.
• server/test/lib/job/job.test.js:
  • the existing describe('job.wrapper') legacy tests are unchanged,
  • the new describe('job.wrapper with { detached: true }') block carries 3 tests covering success path, failure path, and the secondary log path when job.finish itself rejects,
  • a small additional test on updateProgress asserts it rejects when the job id is unknown — easy win for coverage of a pre-existing branch.

Coverage

• device.destroyStatesFrom.js: 100% statements / lines / functions / branches.
• job.wrapper.js: 100% statements / lines / functions; branch coverage at 75% (one defensive guard if (error && error.toString) is not exercised — identical to the pre-existing situation on master, no new uncovered branch introduced).

Tested manually

• A long-running "from beginning" recalculation on my own production instance now returns the HTTP response immediately and runs as a background job (progress visible in the jobs page) — this is the actual functional motivation for the { detached } mode and the reason this PR is the prerequisite for the follow-ups.

Scope

Diff vs base branch (master):

Area	Insertions	Deletions
server (production)	67	13
server (tests)	148	6
Total	215	19

Files touched:

• server/lib/device/device.destroyStatesFrom.js — +23 / -4 (add optional to parameter)
• server/lib/job/job.wrapper.js — +25 / -13 (factor out shared body, add { detached } option)
• server/test/lib/device/device.destroyStatesFrom.test.js — +87 / 0 (2 new tests for the bounded form)
• server/test/lib/job/job.test.js — +80 / -6 (3 new tests for the detached mode + 1 on updateProgress)

No new file, no new public function, no new prototype binding on DeviceManager or Job.

Summary by CodeRabbit

• New Features

  • Jobs can now run in detached/background mode, enabling non-blocking job execution
  • Device state deletion now supports specifying an optional date range (from and to) for more precise cleanup

• Tests

  • Added comprehensive test coverage for detached job execution and error handling
  • Added test cases for bounded date range state deletion, including edge cases

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR adds time-bounded device state deletion and a detached (fire-and-forget) job wrapper. destroyStatesFrom gains an optional to parameter to limit deletions to an inclusive time range. The job wrapper gains a detached option: when enabled it starts a job, runs the wrapped function in background, updates job status to SUCCESS or FAILED with error details, and logs finalization errors. Tests added for both features.

Changes

Device state destruction between timestamps

Layer / File(s)	Summary
Time-bounded state deletion server/lib/device/device.destroyStatesFrom.js, server/test/lib/device/device.destroyStatesFrom.test.js	destroyStatesFrom accepts an optional upper-bound to date (defaults to new Date()), formats both dates to UTC, and deletes device feature states where created_at falls within the inclusive range. Tests verify in-range deletion and no-op behavior when from > to.

Detached job wrapper with background execution

Layer / File(s)	Summary
Detached execution implementation server/lib/job/job.wrapper.js	Wrapper now accepts { detached = false }. In detached mode it starts the job, invokes the wrapped func in a background async task, marks job SUCCESS or FAILED (with error_type and captured error string), and logs exceptions from job.finish via logger.error. Synchronous behavior preserved when detached is false.
Job wrapper tests and infrastructure server/test/lib/job/job.test.js	Per-test Sinon sandbox, db.Job truncation before/after tests, added logger and stubbed event imports, updateProgress not-found test, and a detached-wrapper test suite that polls for terminal status, asserts success/failure terminal states and error capture, and verifies logger.error is called when job.finish rejects.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

• Pierre-Gilles

Poem

I start the job and hop away,
A silent hare at work all day,
States vanish within the time,
Jobs succeed or fail in rhyme,
Logger keeps watch while I play. 🐇

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title mentions 'extend destroyStatesFrom' and 'wrapper' with 'partial recalc support', which aligns with the actual changes: adding optional to parameter to destroyStatesFrom and detached mode to job.wrapper for energy monitoring features.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

server/test/lib/device/device.destroyStatesBetween.test.js (1)

79-99: ⚡ Quick win

Document inverted range behavior in JSDoc.

The test confirms that when from > to, no states are deleted (silent no-op). While this is tested, it's not documented in the JSDoc. Consider adding a note to clarify this behavior for callers.

📝 Suggested JSDoc enhancement

In server/lib/device/device.destroyStatesBetween.js:

 /**
  * `@description` Destroy states between two dates.
  * `@param` {string} selector - Device feature selector.
  * `@param` {Date} from - The start date (inclusive).
  * `@param` {Date} to - The end date (inclusive).
+ * `@note` If from is after to, no states will be deleted.
  * `@returns` {Promise<void>}
  * `@example`
  * await gladys.device.destroyStatesBetween('kitchen-washer-consumption', new Date('2025-01-01'), new Date());
  */

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@server/test/lib/device/device.destroyStatesBetween.test.js` around lines 79 -
99, The test shows Device.destroyStatesBetween silently no-ops when the 'from'
date is after 'to'; update the JSDoc for the destroyStatesBetween method (in
server/lib/device/device.destroyStatesBetween.js / Device class) to explicitly
document this inverted-range behavior: state deletion will not occur and the
method returns without error when from > to, include parameter descriptions for
'from' and 'to' and a short example or note about the no-op behavior so callers
are aware.

server/lib/device/device.destroyStatesBetween.js (1)

14-14: ⚡ Quick win

Consider adding input type validation.

The function doesn't validate that from and to are Date objects. If invalid types are passed, formatDateInUTC may produce unexpected results or throw unclear errors. Consider adding validation to provide clearer error messages.

🛡️ Suggested input validation

 async function destroyStatesBetween(selector, from, to) {
+  if (!(from instanceof Date) || !(to instanceof Date)) {
+    throw new Error('Parameters from and to must be Date objects');
+  }
   const existing = await db.DeviceFeature.findOne({ where: { selector } });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@server/lib/device/device.destroyStatesBetween.js` at line 14, Add input
validation at the start of destroyStatesBetween: ensure both from and to are
actual Date instances and are valid dates (e.g., instanceof Date and
!isNaN(from.getTime()) / !isNaN(to.getTime())); if validation fails, throw a
clear TypeError (e.g., "destroyStatesBetween: 'from' must be a valid Date") so
callers get an explicit error instead of formatDateInUTC producing unclear
failures; place these checks before any call to formatDateInUTC or other logic
that assumes Date inputs.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@server/lib/device/device.destroyStatesBetween.js`:
- Line 14: Add input validation at the start of destroyStatesBetween: ensure
both from and to are actual Date instances and are valid dates (e.g., instanceof
Date and !isNaN(from.getTime()) / !isNaN(to.getTime())); if validation fails,
throw a clear TypeError (e.g., "destroyStatesBetween: 'from' must be a valid
Date") so callers get an explicit error instead of formatDateInUTC producing
unclear failures; place these checks before any call to formatDateInUTC or other
logic that assumes Date inputs.

In `@server/test/lib/device/device.destroyStatesBetween.test.js`:
- Around line 79-99: The test shows Device.destroyStatesBetween silently no-ops
when the 'from' date is after 'to'; update the JSDoc for the
destroyStatesBetween method (in server/lib/device/device.destroyStatesBetween.js
/ Device class) to explicitly document this inverted-range behavior: state
deletion will not occur and the method returns without error when from > to,
include parameter descriptions for 'from' and 'to' and a short example or note
about the no-op behavior so callers are aware.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d2648a44-22d7-4ca2-bdd2-3be4b9684887

📥 Commits

Reviewing files that changed from the base of the PR and between d8012f5 and 62c36bd.

📒 Files selected for processing (6)

• server/lib/device/device.destroyStatesBetween.js
• server/lib/device/index.js
• server/lib/job/index.js
• server/lib/job/job.wrapper.js
• server/test/lib/device/device.destroyStatesBetween.test.js
• server/test/lib/job/job.test.js

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.81%. Comparing base (d8012f5) to head (600f4ce).
⚠️ Report is 191 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2528   +/-   ##
=======================================
  Coverage   98.80%   98.81%           
=======================================
  Files        1018     1018           
  Lines       18305    18316   +11     
=======================================
+ Hits        18087    18099   +12     
+ Misses        218      217    -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[relativeci]

#4195 Bundle Size — 11.52MiB (0%).

600f4ce(current) vs d8012f5 master#4188(baseline)

Warning

Bundle contains 2 duplicate packages – View duplicate packages

Bundle metrics  no changes

	Current #4195	Baseline #4188
Initial JS	6.5MiB	6.5MiB
Initial CSS	310.6KiB	310.6KiB
Cache Invalidation	0%	0%
Chunks	51	51
Assets	179	179
Modules	1643	1643
Duplicate Modules	21	21
Duplicate Code	0.94%	0.94%
Packages	136	136
Duplicate Packages	2	2

Bundle size by type  no changes

	Current #4195	Baseline #4188
JS	8.4MiB	8.4MiB
IMG	2.68MiB	2.68MiB
CSS	328.45KiB	328.45KiB
Fonts	93.55KiB	93.55KiB
Other	18.82KiB	18.82KiB
HTML	13.58KiB	13.58KiB

Bundle analysis report Branch Terdious:energy-recalc-pr1-utili... Project dashboard

---

^{Generated by RelativeCI Documentation Report issue}

[Pierre-Gilles]

Thanks for separating the PR! I've added my feedback.

I'm not sure whether this was AI-invented or based on your own ideas, but this is typically the kind of change we should try to avoid with AI-assisted PRs: copying large blocks of code that were already working well and could have been extended more incrementally.

In this case, I think a smaller and more targeted change would have made the code easier to review and maintain, probably closer to what an experienced developer would naturally do.

[Terdious]

Thanks for the review @Pierre-Gilles.

I'm not sure whether this was AI-invented or based on your own ideas, but this is typically the kind of change we should try to avoid with AI-assisted PRs: copying large blocks of code that were already working well and could have been extended more incrementally.

On the architectural choice (new functions vs. extending existing ones), this was my own design call, not an AI suggestion. The AI executed within constraints I had set; the strategy itself was deliberate.

When I wrote the original energy PR (#2413) in January, I was operating from years of conversations with you about Gladys stability — your recurring theme that the calculation engine is sensitive and that modifications there carry validation cost.

In this case, I think a smaller and more targeted change would have made the code easier to review and maintain, probably closer to what an experienced developer would naturally do.
I totally agree with that and it would have been my initial choice actually. I was just afraid of getting negative feedback from you for fear of breaking the core (especially with the wrapper used in many places in the code).
Adding new isolated functions (destroyStatesBetween,
wrapperDetached) rather than modifying destroyStatesFrom and wrapper
felt like the safest path: zero impact on existing code, full backward
compatibility by construction, and a smaller blast radius if anything
went wrong. That was my reading of the room.

With hindsight, your suggestion is cleaner here and I agree on the merits.
I will refactor both functions along these lines and deploy the updates quickly.

Thanks again for the time on the review.

[Terdious]

@Pierre-Gilles update on testing:

I've pushed a Docker test image terdious/gladys:energy-monitoring-pr1
running since last night on an isolated copy of my prod DB (I dropped
data older than 2025 to keep the run manageable).

I launched a full cost recalculation (costs only, not consumption) —
completed in 1h30, logs are clean. I compared the recalculated values
against my actual prod month by month and year by year: results match.
The only delta is on yesterday's data, which is expected — it
corresponds to the gap between the DB snapshot and the test instance
startup.

I can let it run another day or two if you'd like more confidence on
the long-run incremental mode, or I can move on to preparing a similar
test image for PR2. Let me know what you prefer.

[Pierre-Gilles]

Je ne suis pas sûr de bien comprendre le besoin ni l’utilité de cette modification, j’avoue.

Tu mentionnes des requêtes HTTP qui bloquent, mais de mon côté il me semblait que ce problème avait déjà été corrigé. Le correctif est disponible ici : https://github.com/GladysAssistant/Gladys/blob/master/server/services/energy-monitoring/api/energy-monitoring.controller.js#L23

L’idée était justement de ne plus attendre la fin du traitement avant de répondre à l’appel API (suppression du await), afin que la requête soit résolue immédiatement.

Du coup, quel est exactement le bug que cette modification est censée résoudre ?

[Pierre-Gilles]

Au passage, s’il reste effectivement un bug côté interface, je préférerais avoir une PR dédiée qui corrige le problème de bout en bout, plutôt qu’une PR qui mélange plusieurs sujets différents :)