Add a cleanup mechanism for old db entries #104

nidhal-labidi · 2024-12-11T14:04:35Z

Checklist

Resolves Implement cleanup mechanism for stale database entries in FlottformDatabase #103
Labelled and assigned a reviewer

Reviewer

I've checked out the code and tested it myself.

Changes

Call the method close (in the classes: FlottformFileInputHost and FlottformTextInputHost) whenever the file/text transfer is done so that the close method in FlottformChannelHost triggers a DELETE request to remove the entry from the signaling server DB.
Implement cleanup mechanism for stale database entries on a regular basis (for now every 30 minutes + an entry is considered stale if it was not updated for more than 25 minutes)

…he channel is closed Signed-off-by: nidhal-labidi <[email protected]>

Signed-off-by: nidhal-labidi <[email protected]>

Narigo · 2024-12-16T08:36:07Z

flottform/server/src/database.ts

+		this.cleanupIntervalId = setInterval(() => {
+			// Loop over all entries and delete the stale ones.
+			const now = Date.now();
+			for (const [endpointId, endpointInfo] of this.map) {
+				const lastUpdated = endpointInfo.lastUpdate;
+				if (now - lastUpdated > this.entryTTL) {
+					this.map.delete(endpointId);
+					console.log(`Cleaned up stale entry: ${endpointId}`);
+				}
+			}
+		}, this.cleanupPeriod);


Suggested change

this.cleanupIntervalId = setInterval(() => {

// Loop over all entries and delete the stale ones.

const now = Date.now();

for (const [endpointId, endpointInfo] of this.map) {

const lastUpdated = endpointInfo.lastUpdate;

if (now - lastUpdated > this.entryTTL) {

this.map.delete(endpointId);

console.log(`Cleaned up stale entry: ${endpointId}`);

}

}

}, this.cleanupPeriod);

const cleanupFn = () => {

const nextStartTime = Date.now() + this.cleanupPeriod;

// Loop over all entries and delete the stale ones.

const now = Date.now();

for (const [endpointId, endpointInfo] of this.map) {

const lastUpdated = endpointInfo.lastUpdate;

if (now - lastUpdated > this.entryTTL) {

this.map.delete(endpointId);

console.log(`Cleaned up stale entry: ${endpointId}`);

}

}

this.cleanupIntervalId = setTimeout(cleanupFn, Math.max(0, Date.now - nextStartTime));

};

this.cleanupIntervalId = setTimeout(cleanupFn, this.cleanupPeriod);

Narigo · 2024-12-16T08:37:37Z

flottform/server/src/database.ts

+	private stopCleanup() {
+		// Clear the interval to stop cleanup
+		if (this.cleanupIntervalId) {
+			clearInterval(this.cleanupIntervalId);


Suggested change

clearInterval(this.cleanupIntervalId);

clearTimeout(this.cleanupIntervalId);

Narigo

As discussed, please add tests for the cleanup

…-entries Signed-off-by: nidhal-labidi <[email protected]>

Signed-off-by: nidhal-labidi <[email protected]>

…formDatabase to allow testing Signed-off-by: nidhal-labidi <[email protected]>

Signed-off-by: nidhal-labidi <[email protected]>

Narigo

Thanks for pushing this! I had a look into it and left a few comments - wondering whether it would be easy to use fake timers to not having to wait for over a second in the unit tests

Narigo · 2024-12-17T23:06:59Z

flottform/server/src/database.spec.ts

+			).rejects.toThrow(
+				/clientKey is wrong: Another peer is already connected and you cannot change this info without the correct key anymore. If you lost your key, initiate a new Flottform connection./i
+			);


Since peerkey isn't used in this error message but I don't see a change to the error itself, was/is this test not working? 🤔

Yes that test was not working since as you've said the string 'peerkey' is not used anymore and is replaced by this string: 'clientKey is wrong: Another peer is already connected and you cannot change this info without the correct key anymore. If you lost your key, initiate a new Flottform connection.'
--> I fixed it in this commit: 481b54b

Narigo · 2024-12-17T23:08:51Z

flottform/server/src/database.spec.ts

 			const infoAfter = await db.getEndpoint({ endpointId });
 			expect(infoBefore).toStrictEqual(infoAfter);
 		});
 	});
+
+	describe('startCleanup()', () => {
+		function sleep(ms: number) {


Just to make totally clear this expects you to wait for it, I would add this:

Suggested change

function sleep(ms: number) {

async function sleep(ms: number): Promise<void> {

But the helper function itself could also go somewhere else and doesn't need to be inside the describe block

Since I changed to fake timers like you suggested, I don't think we need it anymore!

Narigo · 2024-12-17T23:17:13Z

flottform/server/src/database.ts

@@ -25,8 +26,46 @@ function createRandomEndpointId(): string {

 class FlottformDatabase {
 	private map = new Map<EndpointId, EndpointInfo>();
+	private cleanupTimeoutId: NodeJS.Timeout | null = null;
+	private cleanupPeriod: number;
+	private entryTTL: number; // Time-to-Live for each entry


How about entryTimeoutInMs to make it clear we're expecting milliseconds here? Also, TTL gets explained in a comma, so I'd suggest to get rid of it by not using an abbreviation. Abbreviations in general, I'd favor entryTtl, so if we end up having another abbreviation before or after this one, it can stay consistent (HttpId vs HTTPID or something along those lines...)

Narigo · 2024-12-17T23:19:01Z

flottform/server/src/database.ts

+	}
+
+	private cleanupFn() {
+		if (!this.map || this.map.size === 0) return;


I think if we fall down to size 0, it will never call the cleanupFn again. Is there a test for this?

The cleanup mechanism is designed to run periodically, regardless of whether the map is empty at the time of execution. The purpose is to ensure stale entries are regularly deleted, even if none exist during a specific cleanup cycle. This mechanism continues to run until the server hosting the database is stopped.

For example, while the map might be empty during the first cleanup call, it’s possible that by the next cycle, new entries have been added, with some of them becoming stale. Stopping the cleanup when the map is empty will lead to not cleaning up stale entries in subsequent cycles. Therefore, it’s crucial that the cleanup function runs unconditionally.

Narigo · 2024-12-17T23:21:13Z

flottform/server/src/database.ts

+	cleanupPeriod = 30 * 60 * 1000,
+	entryTTL = 25 * 60 * 1000


The default values are here a second time. Can we move them into a constant at the beginning of the file? const DEFAULT_CLEANUP_PERIOD = 30 * 60 * 1000; and const DEFAULT_ENTRY_TIMEOUT_IN_MS = 25 * 60 * 1000, then use

Suggested change

cleanupPeriod = 30 * 60 * 1000,

entryTTL = 25 * 60 * 1000

cleanupPeriod = DEFAULT_CLEANUP_PERIOD,

entryTTL = DEFAULT_ENTRY_TIMEOUT_IN_MS

Narigo · 2024-12-17T23:22:07Z

flottform/server/src/database.spec.ts

+			// Sleep for enough time to trigger the first cleanup
+			await sleep(1100);


Is it possible to use fake timers for this and not actually wait for a second?

…timers Signed-off-by: nidhal-labidi <[email protected]>

Signed-off-by: nidhal-labidi <[email protected]>

Narigo

We need some way of heartbeat from the frontend to keep the connection open or react to a closed channel by the server. This needs changes to the frontend as well

Narigo · 2025-01-14T15:04:55Z

flottform/forms/src/flottform-channel-host.ts


 		try {
-			this.rtcConfiguration.iceServers = await this.fetchIceServers(baseApi);
+			this.rtcConfiguration.iceServers = await this.fetchIceServers(this.baseApi);


Passing this.baseApi shouldn't be necessary if we use this.baseApi in the function instead of a parameter

Narigo · 2025-01-14T15:06:55Z

flottform/server/src/database.ts

@@ -85,12 +138,14 @@ class FlottformDatabase {
 		endpointId,
 		clientKey,
 		session,
-		iceCandidates
+		iceCandidates,
+		lastUpdate = Date.now()


This could be done in the function directly where it's necessary instead of passing an argument (and potentially overwriting it)

Narigo · 2025-01-14T15:08:55Z

flottform/server/src/database.ts

@@ -46,6 +95,7 @@ class FlottformDatabase {
 		if (!entry) {
 			throw Error('Endpoint not found');
 		}
+		entry.lastUpdate = Date.now();
 		const { hostKey: _ignore1, clientKey: _ignore2, ...endpoint } = entry;

 		return endpoint;


The lastUpdate should not be exposed to the client as it should only be used internally for cleanup

Narigo · 2025-01-14T15:09:12Z

flottform/server/src/database.ts

@@ -35,7 +83,8 @@ class FlottformDatabase {
 			hostInfo: {
 				session,
 				iceCandidates: []
-			}
+			},
+			lastUpdate: Date.now()
 		};
 		this.map.set(entry.endpointId, entry);
 		return entry;


This is exposing lastUpdate, see comment below

Narigo · 2025-01-14T15:10:06Z

flottform/server/src/database.spec.ts

+				clientKey,
+				session: answer,
+				iceCandidates: [],
+				lastUpdate: Date.now()


lastUpdate should be taken care of internally only - no need to set it here

Narigo · 2025-01-14T15:10:19Z

flottform/server/src/database.spec.ts

+				clientKey,
+				session: answer,
+				iceCandidates: [],
+				lastUpdate: Date.now()


lastUpdate should be taken care of internally only - no need to set it here

nidhal-labidi added 2 commits December 11, 2024 10:17

feat: delete entries from the DB of the signaling server as soon as t…

d261744

…he channel is closed Signed-off-by: nidhal-labidi <[email protected]>

feat: add automated cleanup mechanism for inactive database entries

12d007a

Signed-off-by: nidhal-labidi <[email protected]>

nidhal-labidi requested a review from Narigo December 11, 2024 14:04

Narigo reviewed Dec 16, 2024

View reviewed changes

nidhal-labidi added 6 commits December 16, 2024 13:31

Merge branch 'origin/main' into feat/add-cleanup-mechanism-for-old-db…

9886d7f

…-entries Signed-off-by: nidhal-labidi <[email protected]>

wip: update old test to match the new error message

481b54b

Signed-off-by: nidhal-labidi <[email protected]>

add optional parameters to the FlottformDatabase constructor

2edce18

Signed-off-by: nidhal-labidi <[email protected]>

wip: add optional parameters for the constructor and methods of Flott…

fce4f55

…formDatabase to allow testing Signed-off-by: nidhal-labidi <[email protected]>

feat: add test for the cleanup mechanism of FlottformDatabase

89195be

Signed-off-by: nidhal-labidi <[email protected]>

feat: use setTimeout instead of setInterval for the cleanup mechanism

6511b0a

Signed-off-by: nidhal-labidi <[email protected]>

nidhal-labidi requested a review from Narigo December 17, 2024 11:12

Narigo reviewed Dec 17, 2024

View reviewed changes

update the properties of FlottformDatabase, update tests to use fake …

9c9b49e

…timers Signed-off-by: nidhal-labidi <[email protected]>

nidhal-labidi requested a review from Narigo December 18, 2024 10:18

update the constructor of FlottformDatabase and tests

742aa3d

Signed-off-by: nidhal-labidi <[email protected]>

Narigo requested changes Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a cleanup mechanism for old db entries #104

Add a cleanup mechanism for old db entries #104

nidhal-labidi commented Dec 11, 2024

Narigo Dec 16, 2024

Narigo Dec 16, 2024

Narigo left a comment

Narigo left a comment

Narigo Dec 17, 2024

nidhal-labidi Dec 18, 2024

Narigo Dec 17, 2024

nidhal-labidi Dec 18, 2024

Narigo Dec 17, 2024

Narigo Dec 17, 2024

nidhal-labidi Dec 18, 2024

Narigo Dec 17, 2024

Narigo Dec 17, 2024

Narigo left a comment

Narigo Jan 14, 2025

Narigo Jan 14, 2025

Narigo Jan 14, 2025

Narigo Jan 14, 2025

Narigo Jan 14, 2025

Narigo Jan 14, 2025

	clearInterval(this.cleanupIntervalId);
	clearTimeout(this.cleanupIntervalId);

	function sleep(ms: number) {
	async function sleep(ms: number): Promise<void> {

		// Sleep for enough time to trigger the first cleanup
		await sleep(1100);

Add a cleanup mechanism for old db entries #104

Are you sure you want to change the base?

Add a cleanup mechanism for old db entries #104

Conversation

nidhal-labidi commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narigo left a comment

Choose a reason for hiding this comment

Narigo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narigo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment