Clean up related database rows upon device and project deletion #6424

Steve-Mcl · 2025-12-16T09:29:09Z

Description

Remove MCPRegistration entries on device and project deletion

UPDATE: Following the discovery ([#6466(https://github.com//issues/6466)) made during the implementation of this PR - that other models were not being cleaned up too, this PR has been updated to handle/clean up all soft relations (MCPRegistrations, AccessToken, DeviceSettings, BrokerClient, AuthClient, TeamBrokerClient)

Tests

Added model unit tests to cover this:

`test/unit/forge/ee/db/models/mcp_spec.js` (new tests)

▼ MCPRegistration Model
  ✔ deleting an instance should remove associated MCP entry
  ✔ deleting a device should remove associated MCP entry
  ✔ bulk deleting devices should remove associated MCP entries

`test/unit/forge/db/models/Project_spec.js`

▼ Project model
  ▼ Relations
      ✔ should delete associated table rows on single Project delete (259ms)

`test/unit/forge/db/models/Device_spec.js`

▼ Device model
   ▼ Relations
      ✔ should delete associated table rows on single Device delete (279ms)
      ✔ should delete associated table rows on bulk Device delete (386ms)

NOTE: Other failing tests in test/unit/forge/routes/api/projectSnapshots_spec.js handled too - there was a duplicate after block!

Related Issue(s)

closes #6423
closes #6466

Checklist

Labels

Includes a DB migration? -> add the area:migration label

Steve-Mcl · 2025-12-16T09:41:12Z

bah, postgres test fail...

error: database "flowforge" already exists
ERROR: "test:unit:forge" exited with 1.
ERROR: "test:unit" exited with 1.
Error: Process completed with exit code 1.

@hardillb any thoughts?

…letion

Steve-Mcl · 2025-12-16T10:07:21Z

Added unit tests - lets see if they pass.

hardillb

Withdrawing approval while tests are sorted out

Steve-Mcl · 2025-12-16T12:34:05Z

Ran PG tests locally and I am seeing below errors but I cannot understand why the project afterDestroy hook causes errors:

test/unit/forge/routes/api/project_spec.js

      ✔ Non-owner member cannot delete instance
Error cleaning up MCPRegistrations for deleted project: Error: ConnectionManager.getConnection was called after the connection manager was closed!
    at ConnectionManager.getConnection (flowfuse\node_modules\sequelize\lib\dialects\abstract\connection-manager.js:70:13)        
    at flowfuse\node_modules\sequelize\lib\sequelize.js:305:111
    at flowfuse\node_modules\retry-as-promised\dist\index.js:59:25
    at new Promise (<anonymous>)
    at retryAsPromised (flowfuse\node_modules\retry-as-promised\dist\index.js:48:12)
    at Sequelize.query (flowfuse\node_modules\sequelize\lib\sequelize.js:300:12)
    at PostgresQueryInterface.bulkDelete (flowfuse\node_modules\sequelize\lib\dialects\abstract\query-interface.js:403:33)        
    at MCPRegistration.destroy (flowfuse\node_modules\sequelize\lib\model.js:1838:42)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async Project.afterDestroy (flowfuse\forge\db\models\Project.js:246:25)
    at async Project.runHooks (flowfuse\node_modules\sequelize\lib\hooks.js:96:7)
    at async model.destroy (flowfuse\node_modules\sequelize\lib\model.js:2630:7)
    at async Object.<anonymous> (flowfuse\forge\routes\api\project.js:283:13)
      ✔ Owner can delete an instance (822ms)
Error cleaning up MCPRegistrations for deleted project: Error: ConnectionManager.getConnection was called after the connection manager was closed!
    at ConnectionManager.getConnection (flowfuse\node_modules\sequelize\lib\dialects\abstract\connection-manager.js:70:13)        
    at flowfuse\node_modules\sequelize\lib\sequelize.js:305:111
    at flowfuse\node_modules\retry-as-promised\dist\index.js:59:25
    at new Promise (<anonymous>)
    at retryAsPromised (flowfuse\node_modules\retry-as-promised\dist\index.js:48:12)
    at Sequelize.query (flowfuse\node_modules\sequelize\lib\sequelize.js:300:12)
    at PostgresQueryInterface.bulkDelete (flowfuse\node_modules\sequelize\lib\dialects\abstract\query-interface.js:403:33)        
    at MCPRegistration.destroy (flowfuse\node_modules\sequelize\lib\model.js:1838:42)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async Project.afterDestroy (flowfuse\forge\db\models\Project.js:246:25)
    at async Project.runHooks (flowfuse\node_modules\sequelize\lib\hooks.js:96:7)
    at async model.destroy (flowfuse\node_modules\sequelize\lib\model.js:2630:7)
    at async Object.<anonymous> (flowfuse\forge\routes\api\project.js:283:13)
      ✔ Handles trying to delete an already deleted container without error (564ms)

What I did note was a similar error was occurring in test/unit/forge/routes/api/projectSnapshots_spec.js and that was (it seems) due to a duplicate after( test clean up handler.

ref: flowfuse unit/forge/routes/api/projectSnapshots_spec.js line 74 and line 89 duplicate handlers

Some kinda weird timing issue that I am not confident are just test related.

Switching to draft for now.

codecov · 2025-12-16T12:53:30Z

Codecov Report

❌ Patch coverage is 95.83333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 76.45%. Comparing base (5d6c972) to head (5c0bc48).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
forge/db/models/Device.js	95.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6424      +/-   ##
==========================================
+ Coverage   76.42%   76.45%   +0.02%     
==========================================
  Files         396      396              
  Lines       19885    19909      +24     
  Branches     4766     4770       +4     
==========================================
+ Hits        15198    15221      +23     
- Misses       4687     4688       +1

Flag	Coverage Δ
backend	`76.45% <95.83%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 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.

Steve-Mcl · 2025-12-16T13:02:33Z

This weird!

hardillb · 2025-12-16T16:37:21Z

@Steve-Mcl happy for me to review this again, or want to sleep on it?

Steve-Mcl · 2025-12-16T20:06:55Z

Not sure!

The try catch is containing things, I just dont like that I have to do that.

I mean tests are passing now!

WDYT?

Would be nice to avoid orphaned MCP entries!

… tests

…eted

…s are removed

Steve-Mcl · 2026-01-06T15:11:36Z

ok, after a LOT of digging and debugging and logging, I have determined our test setups do NOT properly re-initialise the app.db.xxxx models correctly - such that generating a new app = setup() results in the app.db.models.<model> being an instance belonging to the previous app in the previous describe block.

This has caused havoc in calling app.db.models.MCPRegistration.destroy in an afterDestroy hook (normally the hooks call M.<model>.destroy but in the case of MCPRegistration rows being cleaned up in device deletion this is not possible: M does NOT contain EE models (only CE models)!

This alone took a long time to dig down into (mostly due to the obscure error) but even once discovering it, handling it is not so straight forwards. There are several factors that determine whether this will be an issue in a test:. was EE models loaded in a prior describe/it block? did the tests cause a device or instance deletion? was the app object created more than once in a test file?

I did try clearing the require cache of affected model (and all forge/) modules but this lead to other issues in the test suites.

I also tried separating tests out (to avoid calling app=setup() more than once but again, other, existing tests failed.

Ideally, calling app = setup() would absolutely generate a 100% clean / new application but it does not and doing so is not as simple as it appears. This is alos evident when app.db.models/<some-ee-model>.<some-function> is mocked in one file and sinon is not restored, the next test suite gets the old instance of app.db.models/<some-ee-model> and it is still mocked.

Therefore, I have reluctantly reverted to including the try..catch wrapper around the calls to app.db.models.MCPRegistration.destroy in the forge/db/models/Project.js and forge/db/models/Device.js hooks since this is an issue that is caused by the structure of our test setup and the creation of multiple apps throughout certain test suites.

hardillb · 2026-01-06T15:33:14Z

forge/db/models/Project.js

Do we need a afterBulkDelete for Projects when the Team is removed?

Not totally necessary at this Time Ben as we dont have UI (and maybe not even backend) for multiple project deletion - at least not that I know.

This is the UI for bulk device

Pretty sure we dont have this for instances.

shoot - I should read what you worte to the end of the sentence!

I did look at the cascades for project deletion and in my head did not think we needed that.

I thought we had to delete the projects before a team could be deleted - perhaps that used to be the case?

Let me re-check

flowfuse/forge/routes/api/team.js

Lines 693 to 705 in 4f67f68

for (const instance of instances) {

try {

await app.containers.remove(instance)

} catch (err) {

if (err?.statusCode !== 404) {

throw err

}

}

await instance.destroy()

await app.auditLog.Team.project.deleted(request.session.User, null, request.team, instance)

await app.auditLog.Project.project.deleted(request.session.User, null, request.team, instance)

}

Looks like we delete one at time in a loop

@hardillb I checked and team deletion calls each instance and device to be feleted in a loop (therefore calling the singular afterDestroy hooks (ie no need for afterBulkDestroy on the project model)

flowfuse/forge/routes/api/team.js

Lines 693 to 723 in 5c0bc48

for (const instance of instances) {

try {

await app.containers.remove(instance)

} catch (err) {

if (err?.statusCode !== 404) {

throw err

}

}

await instance.destroy()

await app.auditLog.Team.project.deleted(request.session.User, null, request.team, instance)

await app.auditLog.Project.project.deleted(request.session.User, null, request.team, instance)

}

}

// Delete Applications

const applications = await app.db.models.Application.byTeam(request.team.hashid)

for (const application of applications) {

await application.destroy()

await app.auditLog.Team.application.deleted(request.session.User, null, request.team, application)

}

// Delete Devices

const where = {

TeamId: request.team.id

}

const devices = await app.db.models.Device.getAll({}, where, { includeInstanceApplication: true })

for (const device of devices.devices) {

await device.destroy()

await app.auditLog.Team.team.device.deleted(request.session.User, null, request.team, device)

}

dang it, I am now thinking we should be doing it the slow loop way (for device bulk deletion) instead of bulk ops!

@knolleary in Bens absence, could you spare 10m brain cycles to discuss this?

hardillb · 2026-01-06T15:42:43Z

Will let you merge @Steve-Mcl

Steve-Mcl · 2026-01-08T15:07:11Z

@knolleary there is an open question on this PR.

TL;DR;

We currently do app.models.Devices.destroy({ where: { id: list_of_devices } ) in our bulk ops. This means we have to have an afterBulkDestroy on the device model to clean up related records.

In the code for team deletion, we iterate the devices and call device.destroy() meaning cleanup of soft relatives is done via a (new) afterBulkDestroy hook.

This works but only if there where clause has an array of IDs - and for now, thats fine since there is only 1 place in application logic where bulk device deletion is performed and it applies an array of IDs to the query! (tests are a different matter - they sometimes do app.models.Devices.destroy({ where: { } }))

The question is, should we retro device bulk deletion code to iterate devices and call device.destroy() (and remove the afterBulkDestroy hook code) - or - ship this (to get a handle on orphans) and / or raise a follow up to remove the bulk operations?

Remove MCPRegistration entries on device and project deletion

e0fd4c9

Steve-Mcl requested a review from hardillb December 16, 2025 09:29

Steve-Mcl temporarily deployed to staging December 16, 2025 09:32 — with GitHub Actions Inactive

hardillb approved these changes Dec 16, 2025

View reviewed changes

Add tests to ensure MCP entries are removed on instance and device de…

ea0f480

…letion

Steve-Mcl temporarily deployed to staging December 16, 2025 10:10 — with GitHub Actions Inactive

Steve-Mcl added 2 commits December 16, 2025 10:45

Fix MCPRegistration deletion tests

dae743d

Merge branch 'main' into 6423-delete-mcp-on-model-deletion

c381e95

Steve-Mcl temporarily deployed to staging December 16, 2025 10:49 — with GitHub Actions Inactive

Enhance MCP entry validation to ensure 'id' and 'teamId' are strings

3138b21

Steve-Mcl temporarily deployed to staging December 16, 2025 10:55 — with GitHub Actions Inactive

prevent DB close while hooks are running

b6aea14

Steve-Mcl temporarily deployed to staging December 16, 2025 11:44 — with GitHub Actions Inactive

hardillb marked this pull request as draft December 16, 2025 12:15

hardillb requested changes Dec 16, 2025

View reviewed changes

Steve-Mcl added 2 commits December 16, 2025 12:39

duplicate after hook in tests causing odd test fails

109bea0

wrap MCPRegistration deletions in try catch

c5352e3

Steve-Mcl temporarily deployed to staging December 16, 2025 12:44 — with GitHub Actions Inactive

Merge branch 'main' into 6423-delete-mcp-on-model-deletion

7c19d72

Steve-Mcl temporarily deployed to staging January 5, 2026 11:18 — with GitHub Actions Inactive

Steve-Mcl added 3 commits January 6, 2026 11:42

Clean up MCPRegistration logic on project deletion

ddf7d7b

clean up MCPRegistration logic on device deletion

6365d89

Add missing bulk relation clean ups for device deletion

c73d22c

Steve-Mcl added 4 commits January 6, 2026 11:48

Add tests to ensure soft relations are cleaned up when device is deleted

508f55f

Stub MCPRegistration.destroy in tests to prevent connection errors in…

8a19c68

… tests

Add tests to ensure soft relations are cleaned up when project is del…

66ea5c6

…eted

Stub MCPRegistration.destroy in tests to prevent connection errors

15dd772

Steve-Mcl changed the title ~~Remove MCPRegistration entries on device and project deletion~~ Clean up related database rows upon device and project deletion Jan 6, 2026

Steve-Mcl temporarily deployed to staging January 6, 2026 11:54 — with GitHub Actions Inactive

Steve-Mcl and others added 3 commits January 6, 2026 12:08

fix test fail due to no-prototype on fastify v5 cookie

c89f5c8

Add test for bulk deletion of devices to ensure associated MCP entrie…

a02e286

…s are removed

Merge branch 'main' into 6423-delete-mcp-on-model-deletion

17db8c1

Steve-Mcl temporarily deployed to staging January 6, 2026 12:18 — with GitHub Actions Inactive

Steve-Mcl added 2 commits January 6, 2026 14:45

Update MCP server listing test to validate all server entries

8275ef8

Prevent errors during device/instance deletion in tests

dd4f272

Steve-Mcl temporarily deployed to staging January 6, 2026 14:50 — with GitHub Actions Inactive

Restore sinon stubs after tests in Device and Project models

5c0bc48

Steve-Mcl temporarily deployed to staging January 6, 2026 15:12 — with GitHub Actions Inactive

Steve-Mcl marked this pull request as ready for review January 6, 2026 15:29

Steve-Mcl requested a review from hardillb January 6, 2026 15:29

hardillb reviewed Jan 6, 2026

View reviewed changes

hardillb approved these changes Jan 6, 2026

View reviewed changes

	for (const instance of instances) {
	try {
	await app.containers.remove(instance)
	} catch (err) {
	if (err?.statusCode !== 404) {
	throw err
	}
	}

	await instance.destroy()
	await app.auditLog.Team.project.deleted(request.session.User, null, request.team, instance)
	await app.auditLog.Project.project.deleted(request.session.User, null, request.team, instance)
	}

Clean up related database rows upon device and project deletion #6424

Are you sure you want to change the base?

Clean up related database rows upon device and project deletion #6424

Uh oh!

Conversation

Steve-Mcl commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Tests

test/unit/forge/ee/db/models/mcp_spec.js (new tests)

test/unit/forge/db/models/Project_spec.js

test/unit/forge/db/models/Device_spec.js

Related Issue(s)

Checklist

Labels

Uh oh!

Steve-Mcl commented Dec 16, 2025

Uh oh!

Steve-Mcl commented Dec 16, 2025

Uh oh!

hardillb left a comment

Choose a reason for hiding this comment

Uh oh!

Steve-Mcl commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Steve-Mcl commented Dec 16, 2025

Uh oh!

hardillb commented Dec 16, 2025

Uh oh!

Steve-Mcl commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Steve-Mcl commented Jan 6, 2026

Uh oh!

hardillb Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Steve-Mcl Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Steve-Mcl Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hardillb Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Steve-Mcl Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Steve-Mcl Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Steve-Mcl Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

hardillb commented Jan 6, 2026

Uh oh!

Steve-Mcl commented Jan 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Steve-Mcl commented Dec 16, 2025 •

edited

Loading

`test/unit/forge/ee/db/models/mcp_spec.js` (new tests)

`test/unit/forge/db/models/Project_spec.js`

`test/unit/forge/db/models/Device_spec.js`

Steve-Mcl commented Dec 16, 2025 •

edited

Loading

codecov bot commented Dec 16, 2025 •

edited

Loading

Steve-Mcl commented Dec 16, 2025 •

edited

Loading

Steve-Mcl Jan 6, 2026 •

edited

Loading