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 <nidhal.labidi@compose.us>

Signed-off-by: nidhal-labidi <nidhal.labidi@compose.us>

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 <nidhal.labidi@compose.us>

Signed-off-by: nidhal-labidi <nidhal.labidi@compose.us>

…formDatabase to allow testing Signed-off-by: nidhal-labidi <nidhal.labidi@compose.us>

Signed-off-by: nidhal-labidi <nidhal.labidi@compose.us>

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

 	});
+
+	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

 	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 <nidhal.labidi@compose.us>

Signed-off-by: nidhal-labidi <nidhal.labidi@compose.us>

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

 		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

+		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

+			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

Narigo · 2025-02-17T10:30:43Z

As discussed, this should also include some kind of heartbeat from the host - basically updating the info every 5 minutes to what it was before, just to keep the lastUpdate until the host disconnects and then this endpoint will be removed after 30 minutes

Signed-off-by: nidhal-labidi <nidhal.labidi@compose.us>

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 <nidhal.labidi@compose.us>

feat: add automated cleanup mechanism for inactive database entries

12d007a

Signed-off-by: nidhal-labidi <nidhal.labidi@compose.us>

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 <nidhal.labidi@compose.us>

wip: update old test to match the new error message

481b54b

Signed-off-by: nidhal-labidi <nidhal.labidi@compose.us>

add optional parameters to the FlottformDatabase constructor

2edce18

Signed-off-by: nidhal-labidi <nidhal.labidi@compose.us>

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

fce4f55

…formDatabase to allow testing Signed-off-by: nidhal-labidi <nidhal.labidi@compose.us>

feat: add test for the cleanup mechanism of FlottformDatabase

89195be

Signed-off-by: nidhal-labidi <nidhal.labidi@compose.us>

feat: use setTimeout instead of setInterval for the cleanup mechanism

6511b0a

Signed-off-by: nidhal-labidi <nidhal.labidi@compose.us>

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 <nidhal.labidi@compose.us>

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 <nidhal.labidi@compose.us>

Narigo requested changes Jan 14, 2025

View reviewed changes

Narigo mentioned this pull request Feb 12, 2025

Tasks that need to be done in order to publish the extension #114

Open

11 tasks

nidhal-labidi added 2 commits February 18, 2025 11:13

fix: the attribute lastUpdate is being taken care of internally only

338636c

Signed-off-by: nidhal-labidi <nidhal.labidi@compose.us>

feat: add a heartbeat function in the host to keep the connection alive

8852cc5

Signed-off-by: nidhal-labidi <nidhal.labidi@compose.us>

nidhal-labidi requested a review from Narigo February 18, 2025 12:01

	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

Uh oh!

Conversation

nidhal-labidi commented Dec 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Narigo left a comment

Choose a reason for hiding this comment

Uh oh!

Narigo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Narigo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Narigo commented Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Narigo commented Feb 17, 2025 •

edited

Loading