Jobs: attach structured data to background jobs, shown in the jobs page - #2652
Closed
Terdious wants to merge 23 commits into
Closed
Jobs: attach structured data to background jobs, shown in the jobs page#2652Terdious wants to merge 23 commits into
Terdious wants to merge 23 commits into
Conversation
PR GladysAssistant#2104 (v4.45.0) moved device states to DuckDB but left two SQLite queries behind: - purgeStatesByFeatureId (triggered when "keep history" is toggled off on a feature) counted and deleted states in SQLite, now empty on migrated installations: it logged "0 states to delete" and never purged the DuckDB states. - The device.destroy guard ("too much states") counted SQLite states, so it never triggered on migrated installations, defeating its purpose of avoiding a blocking delete. Count and delete states in DuckDB (same query device.destroy already uses). SQLite states and aggregates are still purged as leftovers of installations that have not run the migration yet, so the migration cannot re-import states of an already-purged feature. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Deleting a device or disabling a feature history did not always clean the DuckDB states (the per-feature purge counted SQLite states since the DuckDB migration), so installations can carry states which no longer belong to any existing device feature. The Activity endpoint already filters them out defensively, but they consume disk space forever. Add a "purge orphaned DuckDB states" action in the DuckDB migration settings card: it deletes every state whose device_feature_id no longer exists, as a background job. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The jobs page only showed a type, a progress percent and an error. For a purge job there was no way to know what was being purged, how many states were found, nor any report once finished — which made the recent purge regression invisible. Extracted from GladysAssistant#2412 and simplified after the feedback on GladysAssistant#2528: no wrapper change and no detached variant (jobs are already fire-and-forget through the event pattern). Jobs attach structured facts themselves: - job.updateProgress(id, progress, dataPatch) merges optional structured data into job.data; job.finish merges instead of overwriting, so a failure report no longer erases the context. - The job data schema only accepts known keys, and only raw facts (names, counts): the front translates them, so reports work in every language. - The three purge jobs attach their context: target device › feature, DuckDB/SQLite/aggregates counts, orphaned states count. The report stays visible on completed jobs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDuckDB state deletion now includes orphaned-state cleanup, time-sliced purging, structured job progress, expanded deletion checks, background-job wiring, localized frontend display, and coverage for cleanup and job metadata behavior. ChangesDuckDB state cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DeviceManager
participant DeviceFeature
participant DuckDB
participant Job
DeviceManager->>DeviceFeature: load current feature IDs
DeviceManager->>DuckDB: read state date bounds
DeviceManager->>DuckDB: delete orphaned monthly slice
DeviceManager->>Job: persist slice progress and counts
DeviceManager->>DeviceManager: set purge-complete variable
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Computed front-side from created_at/updated_at (finish is the last write), so it works for every job type including historical ones, with an adaptive unit: ms, s, min+s, then h+min. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A running job now shows its live elapsed time (after the first second), ticking every second; finished jobs keep showing their final duration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2652 +/- ##
==========================================
- Coverage 98.99% 98.99% -0.01%
==========================================
Files 1056 1057 +1
Lines 20986 21085 +99
==========================================
+ Hits 20775 20873 +98
- Misses 211 212 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@server/api/routes.js`:
- Around line 212-215: Require admin privileges for the post
/api/v1/device/purge_orphaned_duckdb_states route by adding admin: true
alongside authenticated: true, matching the authorization configuration used by
system-level routes such as vacuum and shutdown.
In `@server/lib/device/device.purgeOrphanedDuckDbStates.js`:
- Around line 12-41: Add tests in device.purgeOrphanedDuckDbStates.test.js
covering both no-feature scenarios in purgeOrphanedDuckDbStates: when featureIds
is empty and the DuckDB count is zero, and when orphaned states exist and the
WHERE-less DELETE executes. Assert the queries omit the WHERE clause, use no
feature-ID parameters, update job progress, and return the expected
orphaned-state count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 597a197c-c6ec-4381-bcc9-daa4a914be6c
📒 Files selected for processing (21)
front/src/config/i18n/de.jsonfront/src/config/i18n/en.jsonfront/src/config/i18n/fr.jsonfront/src/routes/settings/settings-background-jobs/JobList.jsxfront/src/routes/settings/settings-system/SettingsSystemDuckDbMigration.jsxserver/api/controllers/device.controller.jsserver/api/routes.jsserver/lib/device/device.destroy.jsserver/lib/device/device.purgeAllSqliteStates.jsserver/lib/device/device.purgeOrphanedDuckDbStates.jsserver/lib/device/device.purgeStatesByFeatureId.jsserver/lib/device/index.jsserver/lib/job/job.finish.jsserver/lib/job/job.updateProgress.jsserver/models/job.jsserver/test/controllers/device/device.controller.test.jsserver/test/lib/device/device.destroy.test.jsserver/test/lib/device/device.purgeOrphanedDuckDbStates.test.jsserver/test/lib/device/device.purgeStatesByFeatureId.test.jsserver/test/lib/job/job.test.jsserver/utils/constants.js
Feedback from testing on a 16M-state feature: - The job showed nothing while the initial count ran: attach the purge target to the job before counting, and show "States: counting..." in the jobs page meanwhile. - The DuckDB delete was a single statement, so the progress stayed at 0% for minutes and the single DuckDB write connection was held for the whole purge, blocking live state inserts. Delete in up to 20 created_at slices instead: progress moves, and live inserts interleave between slices. (The table has no id column, so slicing is by time range, not LIMIT-ed chunks.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/lib/device/device.purgeStatesByFeatureId.js (1)
90-94: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueEdge case: uniform-time or single-timestamp data makes early slices no-ops.
When
minDate === maxDate(single timestamp)stepInMsis0, so every bounded slice resolves tocreated_at < minDate(deletes nothing) and the terminal unbounded slice deletes all rows in one statement — defeating the connection-release goal for that case. Behavior is still correct, but consider skipping bound generation whenstepInMs === 0(fall back to a single unbounded delete) to avoid needless empty batches.🤖 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.purgeStatesByFeatureId.js` around lines 90 - 94, Handle the zero-duration range in the slice-bound generation used by purgeStatesByFeatureId: when minDate and maxDate produce stepInMs === 0, skip creating bounded slices and use a single unbounded delete batch; retain the existing slicing behavior for positive stepInMs values.
🤖 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.purgeStatesByFeatureId.js`:
- Around line 90-94: Handle the zero-duration range in the slice-bound
generation used by purgeStatesByFeatureId: when minDate and maxDate produce
stepInMs === 0, skip creating bounded slices and use a single unbounded delete
batch; retain the existing slicing behavior for positive stepInMs values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 849545c2-d826-469e-a411-04a5217145a3
📒 Files selected for processing (6)
front/src/config/i18n/de.jsonfront/src/config/i18n/en.jsonfront/src/config/i18n/fr.jsonfront/src/routes/settings/settings-background-jobs/JobList.jsxserver/lib/device/device.purgeStatesByFeatureId.jsserver/lib/device/index.js
✅ Files skipped from review due to trivial changes (3)
- front/src/config/i18n/de.json
- front/src/config/i18n/fr.json
- front/src/config/i18n/en.json
🚧 Files skipped from review as they are similar to previous changes (2)
- front/src/routes/settings/settings-background-jobs/JobList.jsx
- server/lib/device/index.js
Purge jobs now report a validated `step` in their data, displayed while running: waiting for the database (a probe query resolves when the job's turn on the FIFO DuckDB connection arrives — two concurrent purges no longer look frozen), counting states, deleting states, deleting aggregates. On success the counts line switches to "States deleted: ...". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@server/lib/device/device.purgeAllSqliteStates.js`:
- Around line 82-84: The tests for the purge operation only stub updateProgress
without validating its payloads. Update the tests around the purge method to
assert step, sqlite_states_count, and aggregates_count for each updateProgress
call, and add a case with no aggregates to cover the false branch of the
iteratorAggregates.length > 0 guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 524e57f1-3033-44d6-b5a5-544f23a8ebff
📒 Files selected for processing (10)
front/src/config/i18n/de.jsonfront/src/config/i18n/en.jsonfront/src/config/i18n/fr.jsonfront/src/routes/settings/settings-background-jobs/JobList.jsxserver/lib/device/device.purgeAllSqliteStates.jsserver/lib/device/device.purgeOrphanedDuckDbStates.jsserver/lib/device/device.purgeStatesByFeatureId.jsserver/models/job.jsserver/test/lib/device/device.purgeOrphanedDuckDbStates.test.jsserver/test/lib/device/device.purgeStatesByFeatureId.test.js
✅ Files skipped from review due to trivial changes (3)
- front/src/config/i18n/de.json
- front/src/config/i18n/en.json
- front/src/config/i18n/fr.json
🚧 Files skipped from review as they are similar to previous changes (5)
- server/models/job.js
- server/test/lib/device/device.purgeOrphanedDuckDbStates.test.js
- front/src/routes/settings/settings-background-jobs/JobList.jsx
- server/test/lib/device/device.purgeStatesByFeatureId.test.js
- server/lib/device/device.purgeStatesByFeatureId.js
…ries Feedback from testing two concurrent purges on a 448M-state install: - The waiting_database probe could resolve before another job's queued query ran, showing a misleading "counting" step. Submit the probe and the real query in the same tick: the FIFO connection then guarantees the probe resolves only when the query actually runs. - Slicing a small purge is pure overhead (each DELETE is a transaction rewriting row groups): purges below 1M states are now a single statement, bigger ones use at most 10 slices. - Cast the device_feature_id parameters to UUID explicitly, so the comparison cannot fall back to a per-row VARCHAR cast over the whole table; log the connection wait and count durations to measure it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@server/lib/device/device.purgeOrphanedDuckDbStates.js`:
- Around line 29-38: Update the concurrent promise handling in the device purge
flow so countPromise is always observed if probePromise rejects. Ensure the
failure path awaits or otherwise attaches rejection handling to countPromise
before propagating the probe failure, while preserving the existing successful
counting behavior around updateProgress and the COUNT query.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 770a90fb-569d-494a-b5de-d739e100e6db
📒 Files selected for processing (4)
server/lib/device/device.purgeOrphanedDuckDbStates.jsserver/lib/device/device.purgeStatesByFeatureId.jsserver/lib/device/index.jsserver/test/lib/device/device.purgeStatesByFeatureId.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- server/lib/device/index.js
- server/test/lib/device/device.purgeStatesByFeatureId.test.js
- server/lib/device/device.purgeStatesByFeatureId.js
Rework after discussion: instead of a manual button, the cleanup is a one-shot background job started automatically at boot, gated by a system variable (same pattern as the DuckDB migration). The variable is only set after a complete run, so if Gladys restarts mid-purge the job restarts at the next boot — deletes are idempotent. Deliberately slow, as requested: no upfront count (counting orphans over hundreds of millions of states held the read connection for 15-20 minutes during testing), the history is walked in monthly slices with a pause between each, so no DuckDB connection is ever held for long. DuckDB returns the number of deleted rows per statement, so the purged count is accumulated on the fly and reported by the job. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6 tasks
…ured-data # Conflicts: # server/lib/device/device.purgeOrphanedDuckDbStates.js # server/test/lib/device/device.purgeOrphanedDuckDbStates.test.js
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/test/lib/device/device.purgeOrphanedDuckDbStates.test.js (1)
30-91: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMissing test for
featureIds.length === 0branch — still unaddressed.The suite covers the existing-feature path, the no-op flag path, and the empty-table path, but does not test the case where
db.DeviceFeature.findAll()returns an empty array. In that branch,orphanedClauseremains''and all DuckDB states are treated as orphaned and deleted. This is a reachable scenario (e.g., all devices/features removed) and the coding guidelines require 100% patch coverage for server changes.As per coding guidelines: "Assume 100% patch coverage for server changes; test every added branch, error path, helper, and modified line."
🧪 Suggested test
it('should set the flag without purging anything on an empty table', async () => { await db.duckDbWriteConnectionAllAsync('DELETE FROM t_device_feature_state'); const { device, variable } = buildDevice(null); const res = await device.purgeOrphanedDuckDbStates(); expect(res).to.deep.equal({ numberOfOrphanedDuckDbStatesToDelete: 0, }); assert.calledWith(variable.setValue, SYSTEM_VARIABLE_NAMES.DUCKDB_ORPHANED_STATES_PURGED, 'true'); }); + + it('should purge all states when no device features exist', async () => { + // Clear all DeviceFeatures so featureIds is empty and every state is orphaned + await db.DeviceFeature.destroy({ where: {}, truncate: true }); + const { device } = buildDevice(null); + const res = await device.purgeOrphanedDuckDbStates(); + expect(res.numberOfOrphanedDuckDbStatesToDelete).to.equal(5); + const remaining = await db.duckDbReadConnectionAllAsync('SELECT COUNT(*) AS count FROM t_device_feature_state'); + expect(remaining[0].count).to.equal(0); + });🤖 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.purgeOrphanedDuckDbStates.test.js` around lines 30 - 91, Add a test in the device.purgeOrphanedDuckDbStates suite that stubs db.DeviceFeature.findAll() to return an empty array, invokes purgeOrphanedDuckDbStates with the purge flag unset, and verifies all DuckDB states are deleted, the result reports the deleted count, and the purge variable is set to true. Keep the existing setup and assertions for non-empty feature IDs unchanged.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@server/test/lib/device/device.purgeOrphanedDuckDbStates.test.js`:
- Around line 30-91: Add a test in the device.purgeOrphanedDuckDbStates suite
that stubs db.DeviceFeature.findAll() to return an empty array, invokes
purgeOrphanedDuckDbStates with the purge flag unset, and verifies all DuckDB
states are deleted, the result reports the deleted count, and the purge variable
is set to true. Keep the existing setup and assertions for non-empty feature IDs
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecdafcf7-33e7-48be-8db0-4ebde8669681
📒 Files selected for processing (8)
front/src/config/i18n/de.jsonfront/src/config/i18n/en.jsonfront/src/config/i18n/fr.jsonserver/lib/device/device.init.jsserver/lib/device/device.purgeOrphanedDuckDbStates.jsserver/test/lib/device/device.init.test.jsserver/test/lib/device/device.purgeOrphanedDuckDbStates.test.jsserver/utils/constants.js
💤 Files with no reviewable changes (3)
- front/src/config/i18n/fr.json
- front/src/config/i18n/de.json
- front/src/config/i18n/en.json
✅ Files skipped from review due to trivial changes (1)
- server/utils/constants.js
Field test on a 448M-state installation: monthly slices with a fixed 100ms pause kept the CPU/disk saturated for 17 minutes — Gladys stayed up but everything was slow, and each slice held the DuckDB write connection for seconds, delaying live state processing (and scene triggers) accordingly. - Weekly slices instead of monthly: the write connection is never held more than ~1-2s. - Adaptive pause: after each slice, sleep 5x the time the slice took (capped at 60s), so the purge only ever uses ~1/6th of the resources. The one-shot purge takes a few times longer, which does not matter. - Per-slice logs with a greppable prefix (purge-orphaned-duckdb-states) showing dates, deleted count, duration and pause. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ured-data # Conflicts: # server/lib/device/device.purgeOrphanedDuckDbStates.js # server/lib/device/index.js
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mass deletes accumulate delete-tracking memory and WAL until the next checkpoint: flush explicitly at the end so the memory and the disk space are released right away. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Terdious
changed the title
Contributor
Author
|
Field-test results on a 448M-state installation (plus a second tester):
|
The feature list is snapshotted when the purge starts, but the purge runs for a long time by design: states of a feature created while it runs matched the stale NOT IN list in the (previously unbounded) last slice and were deleted. Reported by CodeRabbit. Bound every slice — including the last one — by the purge start date, and clamp the walked range to it too so even future-dated states (skewed device clocks) can never be evaluated against the stale snapshot. States orphaned after the cutoff are handled by the per-feature purge from now on, so nothing is left behind. Also add the missing test for the no-feature-left branch (empty NOT IN clause: every state is orphaned). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ed-data # Conflicts: # server/lib/device/device.purgeOrphanedDuckDbStates.js # server/lib/device/device.purgeStatesByFeatureId.js # server/lib/device/index.js # server/test/lib/device/device.purgeOrphanedDuckDbStates.test.js # server/test/lib/device/device.purgeStatesByFeatureId.test.js
…loads Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7 tasks
Contributor
Pierre-Gilles
added a commit
that referenced
this pull request
Jul 17, 2026
…2672) Co-authored-by: Terdious <thomas.lemaistre76@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope (vs
master)JobList.jsx)job.updateProgress,job.finish,models/job)The purge-job lines are the three purges attaching their structured facts (
step, counts, target names) and, for the per-feature purge, the connection probes and the time-sliced DuckDB delete with live progress. The orphaned purge diff is only its job-data enrichment on top of the version merged in #2651.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:
@duckdb/node-apidriver, all changed lines covered)Did Cypress E2E tests pass?(display-only addition on the jobs page)npm run eslinton front and server)npm run compare-translations—jobsSettings.jobData.*added to en/fr/de)job.updateProgress; REST API unchanged)If you are adding a new features/services which needs explanation, did you modify the user documentation?(self-describing UI)Did you add fake requests data for the demo mode?(no new request)npm run build(Vite) passes locally.Description of change
Problem
The jobs page only shows a type, a progress percent and an error. For a purge job there is no way to know what is being purged, how many states were found, nor any report once finished. Concretely: the per-feature purge was a no-op for 2 years (#2650) and nobody could see it — a job displaying "0 states found (0 DuckDB + 0 SQLite)" would have exposed the bug on day one.
Design — extracted from #2412, simplified after the feedback on #2528
Following the review discussion on #2528: no wrapper change and no detached variant. Jobs are already fire-and-forget through the existing pattern (controller emits an event and answers immediately), and the job knows its own context better than any wrapper — so the job attaches its facts itself:
job.updateProgress(id, progress, dataPatch): optional third parameter, merged intojob.data.job.finishmergesdatainstead of overwriting it: a failure report no longer erases the context attached while running, and the report stays visible on completed jobs.job.dataaccepts a known set of keys (device_name,device_feature_name,duckdb_states_count,sqlite_states_count,aggregates_count,orphaned_states_count) holding names and counts — never sentences. The front translates them (jobsSettings.jobData.*, en/fr/de), so job reports work in every language and older jobs re-render correctly if wordings change.First consumers — the three purge jobs
The jobs page now shows, live and after completion:
Any future job (energy recalculation, backups…) can reuse the same mechanism by adding its keys to the schema and its translations.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes