Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
/*
* Copyright 2026, Polytechnique Montreal and contributors
*
* This file is licensed under the MIT License.
* License text available at https://opensource.org/licenses/MIT
*/
import ProcessManager from './ProcessManager';

const DEFAULT_PORT = 11212; // 11211 is the default memcached port, we use +1 to not clash
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need not to clash? trRouting uses the default port (4000), why does Transition need to have memcached not use the default one? Are servers expected to usually run memcached?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it can be quite common to have a memcached on servers.

const TAG_NAME = 'memcached';

const getServiceName = function (port: number) {
return `memcached${port}`;
};

/**
* Represents a running memcached instance.
* Use this object to get the server connection string and to stop the instance.
*/
export class MemcachedInstance {
constructor(private port: number) {
/* Nothing to do */
}

/**
* Get the server connection string for this memcached instance.
* @returns Server string in format 'localhost:port'
*/
getServer(): string {
return `localhost:${this.port}`;
}

/**
* Check if the memcached instance is running.
* @returns 'running' if the process is running, 'not_running' otherwise
*/
async status(): Promise<'running' | 'not_running'> {
const isRunning = await ProcessManager.isServiceRunning(getServiceName(this.port));
return isRunning ? 'running' : 'not_running';
}

/**
* Stop this memcached instance.
* @returns Status of the stop operation
*/
async stop(): Promise<{ status: 'stopped' | 'not_running' | 'error'; error?: unknown }> {
return ProcessManager.stopProcess(getServiceName(this.port), TAG_NAME);
}
}

/**
* Start a memcached instance.
*
* @param options - Optional configuration
* @param options.port - Port to run memcached on (default: 11212)
* @returns A MemcachedInstance object if started successfully, null otherwise
*/
const start = async (options?: { port?: number }): Promise<MemcachedInstance | null> => {
const port = options?.port ?? DEFAULT_PORT;

const processStatus = await ProcessManager.startProcess({
serviceName: getServiceName(port),
tagName: TAG_NAME,
command: 'memcached',
commandArgs: [
`--port=${port}`,
'--user=nobody', // Memcached does not want to run as root, let's drop to nobody
'-vv' // Enable detailed output for logging
],
waitString: '',
useShell: false,
attemptRestart: false
});

if (processStatus.status === 'started') {
return new MemcachedInstance(port);
}

if (processStatus.status === 'already_running') {
// We treat already_running as an error since we don't know where this memcached
// is managed and we have not idea of its lifecycle. It could disappear at any
// moment.
console.error('a memcached process already exist for port:', port);
} else if (processStatus.status === 'error' && processStatus.error?.code === 'ENOENT') {
console.error('memcached executable does not exist in path');
} else {
console.error('cannot start memcached:', processStatus);
}

return null;
};
Comment on lines +58 to +91
Copy link

@coderabbitai coderabbitai bot Feb 25, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Consider a waitString to avoid a potential startup race.

waitString: '' means start() resolves before memcached has had a chance to print anything, so the returned MemcachedInstance could be handed to TrRouting before the socket is actually accepting connections. Memcached outputs a line like <port> server listening (visible with -vv) that could serve as the sync point.

💡 Suggested change
-        waitString: '',
+        waitString: 'server listening',
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts`
around lines 58 - 91, The start function currently passes waitString: '' to
ProcessManager.startProcess causing a race; update the call in start (the
function creating the MemcachedInstance) to set waitString to the memcached
startup message (include the port) such as "<port> server listening" or a
substring like "server listening" so ProcessManager.startProcess waits until
memcached is accepting connections before returning a started status; keep the
existing -vv flag and ensure the waitString interpolates the port (use the port
variable computed from options or DEFAULT_PORT) so the returned
MemcachedInstance is only created after the socket is ready.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would need to confirm, but I think the waitstring was causing a problem. We would need to check the original version of this code.

Copy link

Choose a reason for hiding this comment

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

🧩 Analysis chain

@greenscientist, I'll search the git history to check the original memcached management code and see what waitString was used previously.


🏁 Script executed:

#!/bin/bash
# Search git log for memcached-related commits and changes
git log --all --oneline --grep="memcached" -20

# Look at the git history of TrRoutingProcessManager.ts to see previous memcached code
git log --oneline -20 -- packages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.ts

# Show the diff of the most recent changes to TrRoutingProcessManager.ts in this PR
git diff origin/main...HEAD -- packages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.ts

Length of output: 5499


🏁 Script executed:

#!/bin/bash
# Look at the full commit history of the memcached-related changes
git log --all --grep="memcached" --format="%H %s" | head -10 | while read hash subject; do
  echo "=== Commit: $subject ==="
  git show "$hash" --stat
  echo ""
done

# Search for any mentions of waitString in memcached-related commits
git log --all -p --grep="memcached" -S "waitString" | head -100

# Check if there are any related issues or comments about memcached startup
rg -i "waitstring|wait.string" --type ts -C 3 packages/chaire-lib-backend/src/utils/processManagers/

Length of output: 58319


@greenscientist, I checked the git history and confirmed that the original memcached implementation in TrRoutingProcessManager also used waitString: '' (empty string). The current MemcachedProcessManager maintains the same approach, so it's consistent with what was working before.

The commit history shows issues were related to useShell and parameter quotation marks, but not the waitString itself. Since the empty waitString approach was proven in production, I'll defer to that experience. If you've found that using a non-empty waitString caused problems in earlier testing, then keeping it empty makes sense.


export default { start };
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ type TrRoutingStartParameters = {
cacheDirectoryPath?: string;
};

type TrRoutingBatchStartParameters = TrRoutingStartParameters & {
memcachedServer?: string;
};

const getServiceName = function (port: number) {
return `trRouting${port}`;
};
Expand All @@ -38,7 +42,7 @@ const startTrRoutingProcess = async (
}: {
debug?: boolean;
cacheDirectoryPath?: string;
cacheAllScenarios?: boolean; // Flag to enable the trRouting connection cache for all scenario`
cacheAllScenarios?: boolean; // Flag to enable the trRouting connection cache for all scenario
logFiles: TrRoutingConfig['logs'];
memcachedServer?: string; // If defined, enable use of memcached and use the value as the server URL
}
Expand All @@ -64,7 +68,7 @@ const startTrRoutingProcess = async (
commandArgs.push(`--cachePath=${directoryManager.projectDirectory}/cache/${config.projectShortname}`);
}

// Set the threads argment if we have a threadCount param and it's higher than one
// Set the threads argument if we have a threadCount param and it's higher than one
if (threadCount && threadCount > 1) {
commandArgs.push(`--threads=${threadCount}`);
}
Expand Down Expand Up @@ -189,8 +193,9 @@ const startBatch = async function (
{
port = serverConfig.getTrRoutingConfig('batch').port,
cacheDirectoryPath = undefined,
debug = undefined
}: TrRoutingStartParameters = {}
debug = undefined,
memcachedServer = undefined
}: TrRoutingBatchStartParameters = {}
) {
// Ensure we don't use more CPU than configured
const maxThreadCount = config.maxParallelCalculators;
Expand All @@ -206,35 +211,6 @@ const startBatch = async function (
const cacheAllScenarios = batchTrRoutingConfig.cacheAllScenarios;
const debugFromParamOrConfig = debug !== undefined ? debug : batchTrRoutingConfig.debug;

// Start Memcached
// TODO This is a placeholder variable to be used when we implement config options to control the use of memcached
// We could add options to enable/disable and to set a specific host/port
const startMemcached = true;
let memcachedServer;
if (startMemcached) {
const memcachedPort = 11212; // 11211 is the default memcached port, we use +1 to not clash
const memcachedProcessStatus = await ProcessManager.startProcess({
serviceName: 'memcached',
tagName: 'memcached',
command: 'memcached',
commandArgs: [
`--port=${memcachedPort}`,
'--user=nobody', // Memcached does not want to run as root, let's drop to nobody
'-vv'
], // Enable detailled output for logging
waitString: '',
useShell: false,
attemptRestart: false
});
if (memcachedProcessStatus.status === 'error' && memcachedProcessStatus.error.code === 'ENOENT') {
console.error('memcached executable does not exist in path');
} else if (memcachedProcessStatus.status === 'started') {
memcachedServer = 'localhost:11212';
} else {
console.error('cannot start memcached:', memcachedProcessStatus);
}
}

const params = {
cacheDirectoryPath: cacheDirectoryPath,
cacheAllScenarios,
Expand All @@ -255,10 +231,6 @@ const startBatch = async function (
const stopBatch = async function (port: number = serverConfig.getTrRoutingConfig('batch').port) {
await stop({ port: port });

// TODO Consider keeping memcached around, to speed up later calculation that might use
// the same data
await ProcessManager.stopProcess('memcached', 'memcached');

return {
status: 'stopped',
service: 'trRoutingBatch',
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
/*
* Copyright 2026, Polytechnique Montreal and contributors
*
* This file is licensed under the MIT License.
* License text available at https://opensource.org/licenses/MIT
*/
import MemcachedProcessManager, { MemcachedInstance } from '../MemcachedProcessManager';
import ProcessManager from '../ProcessManager';

jest.mock('../ProcessManager', () => ({
startProcess: jest.fn(),
stopProcess: jest.fn(),
isServiceRunning: jest.fn()
}));

const startProcessMock = ProcessManager.startProcess as jest.MockedFunction<typeof ProcessManager.startProcess>;
const stopProcessMock = ProcessManager.stopProcess as jest.MockedFunction<typeof ProcessManager.stopProcess>;
const isServiceRunningMock = ProcessManager.isServiceRunning as jest.MockedFunction<typeof ProcessManager.isServiceRunning>;

beforeEach(() => {
jest.clearAllMocks();
});

describe('MemcachedProcessManager.start', () => {
test.each([
{
name: 'default port',
options: undefined,
mockReturn: { status: 'started', service: 'memcached', name: 'memcached' },
expectedServer: 'localhost:11212',
expectedServiceName: 'memcached11212',
expectedCommandArgs: ['--port=11212', '--user=nobody', '-vv']
},
{
name: 'custom port',
options: { port: 11300 },
mockReturn: { status: 'started', service: 'memcached', name: 'memcached' },
expectedServer: 'localhost:11300',
expectedServiceName: 'memcached11300',
expectedCommandArgs: ['--port=11300', '--user=nobody', '-vv']
}
])('should start memcached with $name', async ({ options, mockReturn, expectedServer, expectedServiceName, expectedCommandArgs }) => {
startProcessMock.mockResolvedValue(mockReturn);

const instance = await MemcachedProcessManager.start(options);

expect(instance).not.toBeNull();
expect(instance?.getServer()).toBe(expectedServer);
expect(startProcessMock).toHaveBeenCalledTimes(1);
expect(startProcessMock).toHaveBeenCalledWith(
expect.objectContaining({
serviceName: expectedServiceName,
tagName: 'memcached',
command: 'memcached',
commandArgs: expectedCommandArgs,
waitString: '',
useShell: false,
attemptRestart: false
})
);
});

test.each([
{
name: 'memcached executable not found',
mockReturn: { status: 'error', service: 'memcached', name: 'memcached', error: { code: 'ENOENT' } },
expectedErrorMessage: 'memcached executable does not exist in path'
},
{
name: 'other error',
mockReturn: { status: 'error', service: 'memcached', name: 'memcached', error: { message: 'Some error' } },
expectedErrorMessage: 'cannot start memcached:'
},
{
name: 'memcached is already running',
mockReturn: { status: 'already_running', service: 'memcached', name: 'memcached', error: { message: 'already running' } },
expectedErrorMessage: 'a memcached process already exist for port:'
}
])('should return null when $name', async ({ mockReturn, expectedErrorMessage }) => {
const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation();
startProcessMock.mockResolvedValue(mockReturn);

const instance = await MemcachedProcessManager.start();

expect(instance).toBeNull();
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect.stringContaining(expectedErrorMessage),
...(expectedErrorMessage.includes(':') ? [expect.anything()] : [])
);
consoleErrorSpy.mockRestore();
});
Comment on lines +63 to +91
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Fragile console.error argument detection — consider making it explicit per case.

The heuristic expectedErrorMessage.includes(':') ? [expect.anything()] : [] will silently mis-assert if a future message gains or loses a colon. Adding an explicit extraArgs field to each table entry is clearer.

♻️ Suggested refactor
     test.each([
         {
             name: 'memcached executable not found',
             mockReturn: { status: 'error', service: 'memcached', name: 'memcached', error: { code: 'ENOENT' } },
-            expectedErrorMessage: 'memcached executable does not exist in path'
+            expectedErrorMessage: 'memcached executable does not exist in path',
+            extraArgs: []
         },
         {
             name: 'other error',
             mockReturn: { status: 'error', service: 'memcached', name: 'memcached', error: { message: 'Some error' } },
-            expectedErrorMessage: 'cannot start memcached:'
+            expectedErrorMessage: 'cannot start memcached:',
+            extraArgs: [expect.anything()]
         },
         {
             name: 'memcached is already running',
             mockReturn: { status: 'already_running', ... },
-            expectedErrorMessage: 'a memcached process already exist for port:'
+            expectedErrorMessage: 'a memcached process already exist for port:',
+            extraArgs: [expect.anything()]
         }
-    ])('should return null when $name', async ({ mockReturn, expectedErrorMessage }) => {
+    ])('should return null when $name', async ({ mockReturn, expectedErrorMessage, extraArgs }) => {
         // ...
         expect(consoleErrorSpy).toHaveBeenCalledWith(
             expect.stringContaining(expectedErrorMessage),
-            ...(expectedErrorMessage.includes(':') ? [expect.anything()] : [])
+            ...extraArgs
         );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test.each([
{
name: 'memcached executable not found',
mockReturn: { status: 'error', service: 'memcached', name: 'memcached', error: { code: 'ENOENT' } },
expectedErrorMessage: 'memcached executable does not exist in path'
},
{
name: 'other error',
mockReturn: { status: 'error', service: 'memcached', name: 'memcached', error: { message: 'Some error' } },
expectedErrorMessage: 'cannot start memcached:'
},
{
name: 'memcached is already running',
mockReturn: { status: 'already_running', service: 'memcached', name: 'memcached', error: { message: 'already running' } },
expectedErrorMessage: 'a memcached process already exist for port:'
}
])('should return null when $name', async ({ mockReturn, expectedErrorMessage }) => {
const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation();
startProcessMock.mockResolvedValue(mockReturn);
const instance = await MemcachedProcessManager.start();
expect(instance).toBeNull();
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect.stringContaining(expectedErrorMessage),
...(expectedErrorMessage.includes(':') ? [expect.anything()] : [])
);
consoleErrorSpy.mockRestore();
});
test.each([
{
name: 'memcached executable not found',
mockReturn: { status: 'error', service: 'memcached', name: 'memcached', error: { code: 'ENOENT' } },
expectedErrorMessage: 'memcached executable does not exist in path',
extraArgs: []
},
{
name: 'other error',
mockReturn: { status: 'error', service: 'memcached', name: 'memcached', error: { message: 'Some error' } },
expectedErrorMessage: 'cannot start memcached:',
extraArgs: [expect.anything()]
},
{
name: 'memcached is already running',
mockReturn: { status: 'already_running', service: 'memcached', name: 'memcached', error: { message: 'already running' } },
expectedErrorMessage: 'a memcached process already exist for port:',
extraArgs: [expect.anything()]
}
])('should return null when $name', async ({ mockReturn, expectedErrorMessage, extraArgs }) => {
const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation();
startProcessMock.mockResolvedValue(mockReturn);
const instance = await MemcachedProcessManager.start();
expect(instance).toBeNull();
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect.stringContaining(expectedErrorMessage),
...extraArgs
);
consoleErrorSpy.mockRestore();
});
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@packages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.ts`
around lines 63 - 91, The test's assertion uses a brittle colon-based heuristic
to decide extra console.error args; update the table of cases in
MemcachedProcessManager.test.ts to include an explicit extraArgs (e.g., [] or
[expect.anything()]) property per entry, then replace the ternary expression in
the assertion with that explicit extraArgs (use startProcessMock and
MemcachedProcessManager.start as before and keep consoleErrorSpy). This ensures
each case (the ENOENT, generic error, and already_running scenarios) specifies
its expected additional console.error arguments explicitly instead of relying on
expectedErrorMessage.includes(':').

});

describe('MemcachedInstance', () => {
let instance: MemcachedInstance;

beforeEach(async () => {
startProcessMock.mockResolvedValue({
status: 'started',
service: 'memcached',
name: 'memcached'
});
instance = (await MemcachedProcessManager.start())!;
});

test('getServer should return correct server string', () => {
expect(instance.getServer()).toBe('localhost:11212');
});

test.each([
{ isRunning: true, expectedStatus: 'running' },
{ isRunning: false, expectedStatus: 'not_running' }
])('status should return $expectedStatus when process is $expectedStatus', async ({ isRunning, expectedStatus }) => {
isServiceRunningMock.mockResolvedValue(isRunning);

const status = await instance.status();

expect(status).toBe(expectedStatus);
expect(isServiceRunningMock).toHaveBeenCalledWith('memcached11212');
});

test.each([
{ mockStatus: 'stopped', expectedStatus: 'stopped' },
{ mockStatus: 'not_running', expectedStatus: 'not_running' },
{ mockStatus: 'error', expectedStatus: 'error' }
])('stop should handle $mockStatus status', async ({ mockStatus, expectedStatus }) => {
stopProcessMock.mockResolvedValue({
status: mockStatus,
service: 'memcached',
name: 'memcached'
});

const result = await instance.stop();

expect(result.status).toBe(expectedStatus);
expect(stopProcessMock).toHaveBeenCalledWith('memcached11212', 'memcached');
});
});
Loading
Loading