Skip to content

feat: implement timeout handling for Redis key retrieval #1472

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Artuomka merged 1 commit into from
Dec 15, 2025
Merged

feat: implement timeout handling for Redis key retrieval #1472

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 35 additions & 2 deletions shared-code/src/data-access-layer/data-access-objects/data-access-object-redis.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -815,7 +815,7 @@ export class DataAccessObjectRedis extends BasicDataAccessObject implements IDat
public async getTablesFromDB(): Promise<Array<TableDS>> {
const redisClient = await this.getClient();

const allKeys = await redisClient.keys('*');
const allKeys = await this.getAllKeysWithTimeout(redisClient);
const prefixedTableNames = new Set<string>();
const standaloneKeys: Array<{ key: string; type: string }> = [];

Expand All @@ -833,7 +833,7 @@ export class DataAccessObjectRedis extends BasicDataAccessObject implements IDat
continue;
}
}
}
}
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is trailing whitespace at the end of this line.

Suggested change
}
}

Copilot uses AI. Check for mistakes.

const tables: Array<TableDS> = [];

Expand Down Expand Up @@ -1634,4 +1634,37 @@ export class DataAccessObjectRedis extends BasicDataAccessObject implements IDat
}
});
}

private async getAllKeysWithScan(redisClient: RedisClientType, pattern: string = '*'): Promise<string[]> {
const allKeys: string[] = [];
const scanOptions = { MATCH: pattern, COUNT: 1000 };
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic number 1000 for the COUNT option in SCAN is not explained. Consider extracting this to a named constant with documentation explaining why this batch size was chosen for the SCAN operation.

Copilot uses AI. Check for mistakes.
let cursor = '0';

do {
const result = await redisClient.scan(cursor, scanOptions);
cursor = result.cursor.toString();
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The explicit conversion result.cursor.toString() may be unnecessary depending on the Redis client library's return type. The cursor is typically already a string from the SCAN command. Consider verifying the type returned by the Redis client and removing the conversion if redundant.

Suggested change
cursor = result.cursor.toString();
cursor = result.cursor;

Copilot uses AI. Check for mistakes.
allKeys.push(...result.keys);
} while (cursor !== '0');

Comment on lines +1642 to +1648
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SCAN operation lacks error handling. If any SCAN call fails during iteration, the entire operation will throw an unhandled error. This should be wrapped in try-catch to ensure graceful error handling, especially since this is used as a fallback for when the regular keys() operation times out.

Suggested change
do {
const result = await redisClient.scan(cursor, scanOptions);
cursor = result.cursor.toString();
allKeys.push(...result.keys);
} while (cursor !== '0');
try {
do {
const result = await redisClient.scan(cursor, scanOptions);
cursor = result.cursor.toString();
allKeys.push(...result.keys);
} while (cursor !== '0');
} catch (error) {
// Log the error and return an empty array or the keys collected so far
console.error('Error during Redis SCAN operation:', error);
return [];
}

Copilot uses AI. Check for mistakes.
return allKeys;
}
Comment on lines +1638 to +1650
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new getAllKeysWithScan method lacks test coverage. Given that this is a critical fallback mechanism for timeout scenarios, it should have tests covering: normal iteration with multiple SCAN calls, handling of empty results, and proper cursor management.

Copilot uses AI. Check for mistakes.
Comment on lines +1638 to +1650
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getAllKeysWithScan method lacks documentation. It should have a JSDoc comment explaining its purpose, when it's used (as a fallback for timeouts), what the pattern parameter does, and why SCAN is preferred over KEYS in certain scenarios (non-blocking, production-safe).

Copilot uses AI. Check for mistakes.

private async getAllKeysWithTimeout(
redisClient: RedisClientType,
timeoutMs: number = 5000,
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic number 5000 (5 seconds) for the default timeout is not explained. Consider extracting this to a named constant with documentation explaining why this specific timeout value was chosen, or making it configurable based on environment or connection parameters.

Copilot uses AI. Check for mistakes.
): Promise<string[]> {
const keysPromise = redisClient.keys('*');
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getAllKeysWithTimeout method uses a hardcoded '' pattern in redisClient.keys('*'), while the fallback method getAllKeysWithScan accepts a pattern parameter. This inconsistency means if getAllKeysWithScan is enhanced in the future to support different patterns, the timeout path would still use ''. Consider adding a pattern parameter to getAllKeysWithTimeout and passing it to both the keys() call and the getAllKeysWithScan fallback.

Copilot uses AI. Check for mistakes.

const timeoutPromise = new Promise<null>((resolve) => {
setTimeout(() => resolve(null), timeoutMs);
});

const result = await Promise.race([keysPromise, timeoutPromise]);

if (result === null) {
Comment on lines +1656 to +1664
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a race condition in the timeout handling. If the keys() operation completes just after the timeout fires but before Promise.race resolves, both operations could complete nearly simultaneously, potentially causing undefined behavior. The implementation should ensure proper handling of this edge case.

Suggested change
const keysPromise = redisClient.keys('*');
const timeoutPromise = new Promise<null>((resolve) => {
setTimeout(() => resolve(null), timeoutMs);
});
const result = await Promise.race([keysPromise, timeoutPromise]);
if (result === null) {
let timeoutFired = false;
const keysPromise = redisClient.keys('*');
const timeoutPromise = new Promise<null>((resolve) => {
setTimeout(() => {
timeoutFired = true;
resolve(null);
}, timeoutMs);
});
const result = await Promise.race([keysPromise, timeoutPromise]);
if (result === null) {
// Ensure any late rejection of keysPromise is handled
keysPromise.catch(() => {});

Copilot uses AI. Check for mistakes.
return this.getAllKeysWithScan(redisClient);
}

return result;
Comment on lines +1656 to +1668
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timeout mechanism has a critical flaw: when the timeout occurs, the original keys() operation continues running in the background without cancellation. This can lead to resource leaks and unnecessary load on the Redis server. Additionally, if the keys() call fails with an error, it will reject the Promise.race and bypass the timeout fallback to SCAN. Consider wrapping the keysPromise in a try-catch or implementing proper cancellation/cleanup of the timed-out operation.

Suggested change
const keysPromise = redisClient.keys('*');
const timeoutPromise = new Promise<null>((resolve) => {
setTimeout(() => resolve(null), timeoutMs);
});
const result = await Promise.race([keysPromise, timeoutPromise]);
if (result === null) {
return this.getAllKeysWithScan(redisClient);
}
return result;
let timeoutHit = false;
let timeoutHandle: NodeJS.Timeout;
const timeoutPromise = new Promise<null>((resolve) => {
timeoutHandle = setTimeout(() => {
timeoutHit = true;
resolve(null);
}, timeoutMs);
});
try {
const keysPromise = redisClient.keys('*');
const result = await Promise.race([keysPromise, timeoutPromise]);
clearTimeout(timeoutHandle);
if (result === null) {
// Timeout hit, fallback to SCAN
return this.getAllKeysWithScan(redisClient);
}
return result;
} catch (err) {
// If keysPromise rejects, fallback to SCAN
if (timeoutHandle) {
clearTimeout(timeoutHandle);
}
return this.getAllKeysWithScan(redisClient);
}

Copilot uses AI. Check for mistakes.
}
Comment on lines +1652 to +1669
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getAllKeysWithTimeout method lacks documentation. It should have a JSDoc comment explaining the timeout mechanism, the fallback behavior to SCAN when timeout occurs, and the rationale for the default timeout value.

Copilot uses AI. Check for mistakes.
Comment on lines +1652 to +1669
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new getAllKeysWithTimeout method lacks test coverage. Given the complexity of the timeout and race condition handling, it should have tests covering: successful completion before timeout, timeout triggering fallback to SCAN, and error handling when keys() fails.

Copilot uses AI. Check for mistakes.
}
Loading