Move memcached management outside of TrRoutingProcessManager#1720
Move memcached management outside of TrRoutingProcessManager#1720greenscientist wants to merge 1 commit intochairemobilite:mainfrom
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 11 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts">
<violation number="1" location="packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts:70">
P2: Avoid returning a MemcachedInstance with the requested port when the service is already running; the running process may be on a different port, which would give callers an invalid server URL.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts
Outdated
Show resolved
Hide resolved
WalkthroughAdds a MemcachedProcessManager module exposing a MemcachedInstance class and a start(options) function to start/stop/query a memcached process. Refactors TrRoutingProcessManager to accept an optional memcachedServer parameter and removes internal memcached lifecycle from its batch path. Introduces memcachedServer into TrRoutingBatchJobParameters and batch job payloads. Extends TrRoutingBatchManager to accept constructor options with memcachedServer, to start/stop an owned memcached instance when needed, and to pass memcachedServer through to TrRoutingProcessManager. Tests updated/added across process managers and batch managers to mock and verify the new memcached flows. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.ts (1)
86-118: Stop owned memcached if TrRouting start fails.
If memcached starts and TrRouting fails, the memcached process is left running and ownership flags stay set.🐛 Suggested fix
- const startStatus = await TrRoutingProcessManager.startBatch(threadCount, { - cacheDirectoryPath: options?.cacheDirectoryPath, - memcachedServer - }); - - if (startStatus.status !== 'started') { - throw new Error(`Failed to start TrRouting instance: ${startStatus.status}`); - } - - console.log('trRouting multiple startStatus', startStatus); - - return { - threadCount, - port: startStatus.port - }; + try { + const startStatus = await TrRoutingProcessManager.startBatch(threadCount, { + cacheDirectoryPath: options?.cacheDirectoryPath, + memcachedServer + }); + + if (startStatus.status !== 'started') { + throw new Error(`Failed to start TrRouting instance: ${startStatus.status}`); + } + + console.log('trRouting multiple startStatus', startStatus); + + return { + threadCount, + port: startStatus.port + }; + } catch (error) { + if (this.ownsMemcached && this.memcachedInstance) { + await this.memcachedInstance.stop(); + this.memcachedInstance = undefined; + this.ownsMemcached = false; + } + throw error; + }packages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.ts (1)
624-675: Fix test name typo and consider test.each for memcached variants.
Rename “memached” → “memcached”, and consider parameterizing the two memcachedServer scenarios to reduce duplication.✏️ Typo fix
- test('start batch process with just memached parameters set', async () => { + test('start batch process with just memcached parameters set', async () => {As per coding guidelines, please use parametric Jest tests when multiple expects are present.
🤖 Fix all issues with AI agents
In
`@packages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.ts`:
- Around line 24-169: Several tests in MemcachedProcessManager.test.ts repeat
similar setup/assertions (e.g., the start variations and the MemcachedInstance
status/stop cases); convert these to parametric Jest tests using test.each to
reduce duplication. Specifically, collapse the three start scenarios in the
"MemcachedProcessManager.start" describe into a test.each that iterates over
cases (default port, custom port, already_running/error) by supplying
startProcessMock return values and expected commandArgs/server/return assertions
for MemcachedProcessManager.start, and convert the MemcachedInstance status/stop
tests into test.each entries that provide isServiceRunningMock or
stopProcessMock return values and expected status/stop outcomes for
instance.status() and instance.stop(); keep existing mocks
(startProcessMock/isServiceRunningMock/stopProcessMock) and references to
MemcachedProcessManager.start and MemcachedInstance methods to locate the code.
In
`@packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts`:
- Around line 17-19: The MemcachedInstance class currently has an empty
constructor using a private parameter (constructor(private port: number) {})
which triggers the linter; fix it by making the assignment explicit—either
change the constructor signature to expose the field (e.g., constructor(public
readonly port: number)) or add an explicit assignment in the constructor body
(this.port = port) and ensure the class has a matching property declaration;
update the MemcachedInstance constructor accordingly.
In
`@packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts`:
- Around line 156-158: Replace the loose any[] typing for the progress events
with a concrete interface type matching the emitted shape: change the
progressEvents declaration used alongside progressEmitter.on('progress', ...) in
the test (variable progressEvents) to Array<{ name: string; progress: number }>,
and make the same replacement for the other occurrence around lines 210-212 so
the test uses strict typing for the progress event objects.
- Around line 124-139: Replace the multiple-case assertions in the test for
TrRoutingBatchManager.startBatch with a parametric test using test.each: create
an array of input/expected pairs like [{workload:3, expectedThreads:1},
{workload:9, expectedThreads:3}, {workload:100, expectedThreads:4}], then write
test.each over those cases to instantiate TrRoutingBatchManager (using the same
progressEmitter and options) and call manager.startBatch(workload) and assert
mockTrRoutingStartBatch was last called with expectedThreads and
expect.anything(); keep references to TrRoutingBatchManager.startBatch and
mockTrRoutingStartBatch so the test behavior is identical but each case is
isolated.
packages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.ts
Show resolved
Hide resolved
packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts
Show resolved
Hide resolved
| test('should calculate thread count based on workload size', async () => { | ||
| const manager = new TrRoutingBatchManager(progressEmitter, { | ||
| memcachedServer: 'external:11211' | ||
| }); | ||
|
|
||
| // Small workload: ceil(3/3) = 1 | ||
| await manager.startBatch(3); | ||
| expect(mockTrRoutingStartBatch).toHaveBeenLastCalledWith(1, expect.anything()); | ||
|
|
||
| // Medium workload: ceil(9/3) = 3 | ||
| await manager.startBatch(9); | ||
| expect(mockTrRoutingStartBatch).toHaveBeenLastCalledWith(3, expect.anything()); | ||
|
|
||
| // Large workload: min(4, ceil(100/3)) = 4 (capped at maxParallelCalculators) | ||
| await manager.startBatch(100); | ||
| expect(mockTrRoutingStartBatch).toHaveBeenLastCalledWith(4, expect.anything()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read the relevant test
cat -n packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts | sed -n '120,145p'Repository: chairemobilite/transition
Length of output: 1360
Use a parametric test here with test.each.
This test has multiple expect statements. Per coding guidelines, parametric tests should be used to simplify debugging of failed tests.
♻️ Example refactor
-test('should calculate thread count based on workload size', async () => {
- const manager = new TrRoutingBatchManager(progressEmitter, {
- memcachedServer: 'external:11211'
- });
-
- // Small workload: ceil(3/3) = 1
- await manager.startBatch(3);
- expect(mockTrRoutingStartBatch).toHaveBeenLastCalledWith(1, expect.anything());
-
- // Medium workload: ceil(9/3) = 3
- await manager.startBatch(9);
- expect(mockTrRoutingStartBatch).toHaveBeenLastCalledWith(3, expect.anything());
-
- // Large workload: min(4, ceil(100/3)) = 4 (capped at maxParallelCalculators)
- await manager.startBatch(100);
- expect(mockTrRoutingStartBatch).toHaveBeenLastCalledWith(4, expect.anything());
-});
+test.each([
+ { locationsCount: 3, expectedThreads: 1 },
+ { locationsCount: 9, expectedThreads: 3 },
+ { locationsCount: 100, expectedThreads: 4 }
+])('should calculate thread count for $locationsCount locations', async ({ locationsCount, expectedThreads }) => {
+ const manager = new TrRoutingBatchManager(progressEmitter, {
+ memcachedServer: 'external:11211'
+ });
+
+ await manager.startBatch(locationsCount);
+ expect(mockTrRoutingStartBatch).toHaveBeenLastCalledWith(expectedThreads, expect.anything());
+});📝 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.
| test('should calculate thread count based on workload size', async () => { | |
| const manager = new TrRoutingBatchManager(progressEmitter, { | |
| memcachedServer: 'external:11211' | |
| }); | |
| // Small workload: ceil(3/3) = 1 | |
| await manager.startBatch(3); | |
| expect(mockTrRoutingStartBatch).toHaveBeenLastCalledWith(1, expect.anything()); | |
| // Medium workload: ceil(9/3) = 3 | |
| await manager.startBatch(9); | |
| expect(mockTrRoutingStartBatch).toHaveBeenLastCalledWith(3, expect.anything()); | |
| // Large workload: min(4, ceil(100/3)) = 4 (capped at maxParallelCalculators) | |
| await manager.startBatch(100); | |
| expect(mockTrRoutingStartBatch).toHaveBeenLastCalledWith(4, expect.anything()); | |
| test.each([ | |
| { locationsCount: 3, expectedThreads: 1 }, | |
| { locationsCount: 9, expectedThreads: 3 }, | |
| { locationsCount: 100, expectedThreads: 4 } | |
| ])('should calculate thread count for $locationsCount locations', async ({ locationsCount, expectedThreads }) => { | |
| const manager = new TrRoutingBatchManager(progressEmitter, { | |
| memcachedServer: 'external:11211' | |
| }); | |
| await manager.startBatch(locationsCount); | |
| expect(mockTrRoutingStartBatch).toHaveBeenLastCalledWith(expectedThreads, expect.anything()); | |
| }); |
🤖 Prompt for AI Agents
In
`@packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts`
around lines 124 - 139, Replace the multiple-case assertions in the test for
TrRoutingBatchManager.startBatch with a parametric test using test.each: create
an array of input/expected pairs like [{workload:3, expectedThreads:1},
{workload:9, expectedThreads:3}, {workload:100, expectedThreads:4}], then write
test.each over those cases to instantiate TrRoutingBatchManager (using the same
progressEmitter and options) and call manager.startBatch(workload) and assert
mockTrRoutingStartBatch was last called with expectedThreads and
expect.anything(); keep references to TrRoutingBatchManager.startBatch and
mockTrRoutingStartBatch so the test behavior is identical but each case is
isolated.
| test('should emit progress events', async () => { | ||
| const progressEvents: any[] = []; | ||
| progressEmitter.on('progress', (data) => progressEvents.push(data)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the test file at the specified lines
wc -l packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tsRepository: chairemobilite/transition
Length of output: 168
🏁 Script executed:
# Read the test file around the flagged lines
sed -n '150,220p' packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tsRepository: chairemobilite/transition
Length of output: 2775
🏁 Script executed:
# Find the main TrRoutingBatchManager class to understand how progress events are emitted
find packages/transition-backend/src/services/transitRouting -name "*.ts" ! -path "*__tests__*" -type fRepository: chairemobilite/transition
Length of output: 1068
🏁 Script executed:
# Read the TrRoutingBatchManager.ts source
cat -n packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.tsRepository: chairemobilite/transition
Length of output: 7724
🏁 Script executed:
# Search for any interface or type definitions related to progress events
rg -i "progress" packages/transition-backend/src/services/transitRouting/ --type ts -A 3 -B 1 | head -100Repository: chairemobilite/transition
Length of output: 10690
Use a concrete type instead of any[] for the progress events array.
Replace any[] with Array<{ name: string; progress: number }> to match the actual event structure and comply with TypeScript strict typing guidelines.
♻️ Example refactor
-const progressEvents: any[] = [];
+const progressEvents: Array<{ name: string; progress: number }> = [];Also applies to lines 210-212.
🤖 Prompt for AI Agents
In
`@packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts`
around lines 156 - 158, Replace the loose any[] typing for the progress events
with a concrete interface type matching the emitted shape: change the
progressEvents declaration used alongside progressEmitter.on('progress', ...) in
the test (variable progressEvents) to Array<{ name: string; progress: number }>,
and make the same replacement for the other occurrence around lines 210-212 so
the test uses strict typing for the progress event objects.
packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts
Outdated
Show resolved
Hide resolved
| */ | ||
| import ProcessManager from './ProcessManager'; | ||
|
|
||
| const DEFAULT_PORT = 11212; // 11211 is the default memcached port, we use +1 to not clash |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes, it can be quite common to have a memcached on servers.
packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts
Show resolved
Hide resolved
packages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.ts
Outdated
Show resolved
Hide resolved
| * | ||
| * Memcached is handled as follows: | ||
| * 1. If options.memcachedServer is provided, use it | ||
| * 2. Else if constructor options.memcachedServer was provided, use it |
There was a problem hiding this comment.
Do we need both the options parameter in this method and the options in the constructor? Why would one override the options if it is set in the constructor? What use cases do these 2 cases cover?
There was a problem hiding this comment.
Maybe you have a one off case where you already have a TrRoutingBatchManager but want to override . Could have been how I'm planning to use it for the simulations. It's been a while already, trying to remember ;)
There was a problem hiding this comment.
Please try to remember and have a good explanation for why the options here. The way I see it, if you call it with options, after having called it without options, for example, it may override the object's memcachedInstance and ownsMemcached and you may have a rogue memcached instance lost somewhere. You should not be able to call the startBatch function more than once with different options. Plus with the service name without a port, you may end up with an instance running on a different port than the one you want.
| export class TrRoutingBatchManager { | ||
| constructor(private progressEmitter: EventEmitter) { | ||
| private memcachedInstance?: MemcachedInstance; | ||
| private ownsMemcached = false; |
There was a problem hiding this comment.
Do you need both properties? Could it be possible that there is a memcachedInstance, but ownsMemcached is false?
There was a problem hiding this comment.
Yes. The case of the algo genetic, there will be a memcache instance, but it will not be managed by the batchManager
| stop: jest.fn().mockResolvedValue({ status: 'stopped' }) | ||
| }) | ||
| })); | ||
| const mockMemcachedStart = MemcachedProcessManager.start as jest.MockedFunction<typeof MemcachedProcessManager.start>; |
There was a problem hiding this comment.
Since you added the memcached stuff in this test, I would expect some diff that includes the memcached parameters, like there is in the TrRoutingBatch.test.ts file.
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts
Outdated
Show resolved
Hide resolved
greenscientist
force-pushed
the
memcachedlonglife
branch
from
6e4bda5 to
651e212
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.ts (1)
80-121: 🧹 Nitpick | 🔵 TrivialConsider cleanup on startBatch failure.
If
TrRoutingProcessManager.startBatchfails (line 109-111 throws), the just-started memcached instance remains running but the caller may not callstopBatch. The current callers (e.g.,TrAccessibilityMapBatch.tsline 203) do callstopBatchin theirfinallyblock, so this works in practice. But as a defensive measure, stopping the owned memcached in the error path would make the class self-contained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.ts` around lines 80 - 121, In startBatch: if TrRoutingProcessManager.startBatch(...) throws after we started a memcached instance via MemcachedProcessManager.start(), ensure we stop that owned instance before rethrowing; wrap the call to TrRoutingProcessManager.startBatch in a try/catch (or add a nested try/finally) and when an error occurs check this.ownsMemcached and this.memcachedInstance and call the appropriate cleanup (e.g., MemcachedProcessManager.stop(this.memcachedInstance) or this.memcachedInstance.stop()) to stop the memcached instance, clear this.memcachedInstance/this.ownsMemcached, then rethrow the original error.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.tspackages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.tspackages/transition-backend/src/services/transitRouting/BatchAccessibilityMapJob.tspackages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.tspackages/transition-backend/src/services/transitRouting/TrRoutingBatchJobParameters.tspackages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.tspackages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Use TypeScript strictly and avoid
anytypes - maintain type safety throughout the application
Files:
packages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.tspackages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.tspackages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.tspackages/transition-backend/src/services/transitRouting/BatchAccessibilityMapJob.tspackages/transition-backend/src/services/transitRouting/TrRoutingBatchJobParameters.tspackages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.tspackages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
**/*.{ts,tsx}: Use UUIDs as primary keys for most entities in the database
Use the Status/Result pattern from chaire-lib-common for service functions and handlers
Indent using 4 spaces
Do not use trailing spaces, even in comments or documentation; replace lines with only spaces with blank lines
Use parentheses for arrow function parameters
Document non-trivial functions and attributes using JSDoc or inline comments when short
Use Geographic data in GeoJSON format with coordinates as [longitude, latitude]
Use i18next and thet()function for all user-facing strings to support internationalization
Files:
packages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.tspackages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.tspackages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.tspackages/transition-backend/src/services/transitRouting/BatchAccessibilityMapJob.tspackages/transition-backend/src/services/transitRouting/TrRoutingBatchJobParameters.tspackages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.tspackages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests
Files:
packages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.ts
🧠 Learnings (17)
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to **/*.test.{ts,tsx} : Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests
Applied to files:
packages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to **/*.{ts,tsx} : Use the Status/Result pattern from chaire-lib-common for service functions and handlers
Applied to files:
packages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.ts
📚 Learning: 2025-09-25T16:40:55.314Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/services/executableJob/ExecutableJob.ts:152-158
Timestamp: 2025-09-25T16:40:55.314Z
Learning: In packages/transition-backend/src/services/executableJob/ExecutableJob.ts, greenscientist deferred improving the hasFile() method to check actual file existence on disk rather than just metadata, indicating it should be addressed in a later PR.
Applied to files:
packages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.ts
📚 Learning: 2025-09-25T16:09:00.577Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts:42-45
Timestamp: 2025-09-25T16:09:00.577Z
Learning: In packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts, greenscientist deferred adding input file validation using `if (!job.hasInputFile())` and requested to be reminded about it in a later PR.
Applied to files:
packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.tspackages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.tspackages/transition-backend/src/services/transitRouting/BatchAccessibilityMapJob.tspackages/transition-backend/src/services/transitRouting/TrRoutingBatchJobParameters.tspackages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.ts
📚 Learning: 2025-10-20T18:31:07.612Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1516
File: packages/chaire-lib-backend/src/utils/trRouting/TrRoutingServiceBackend.ts:29-41
Timestamp: 2025-10-20T18:31:07.612Z
Learning: In TrRoutingServiceBackend (packages/chaire-lib-backend/src/utils/trRouting/TrRoutingServiceBackend.ts), the fetch-retry package was not working as expected, so custom retry logic was implemented instead.
Applied to files:
packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.tspackages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.tspackages/transition-backend/src/services/transitRouting/BatchAccessibilityMapJob.tspackages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.ts
📚 Learning: 2025-09-25T17:08:56.828Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/tasks/TransitionWorkerPool.ts:105-107
Timestamp: 2025-09-25T17:08:56.828Z
Learning: In packages/transition-backend/src/tasks/TransitionWorkerPool.ts, greenscientist deferred changing the wrapBatchAccessMap input file validation from `hasInputFile()` to `getFilePath('input')` (actual file existence check), indicating it will be addressed in an upcoming file access refactor.
Applied to files:
packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.tspackages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.tspackages/transition-backend/src/services/transitRouting/BatchAccessibilityMapJob.tspackages/transition-backend/src/services/transitRouting/TrRoutingBatchJobParameters.tspackages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.ts
📚 Learning: 2025-11-04T19:35:10.045Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1554
File: packages/transition-common/src/services/simulation/simulationMethod/OdTripSimulationMethod.ts:23-36
Timestamp: 2025-11-04T19:35:10.045Z
Learning: In the OdTripSimulationMethod (packages/transition-common/src/services/simulation/simulationMethod/OdTripSimulationMethod.ts), the team plans to move away from the datasource path and use batch calculations instead. The current non-functional dataSourceId selector will be reworked as part of this architectural change.
Applied to files:
packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.tspackages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.tspackages/transition-backend/src/services/transitRouting/BatchAccessibilityMapJob.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts
📚 Learning: 2025-11-12T14:18:53.288Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1568
File: packages/transition-backend/src/tasks/serverWorkerPool.ts:25-31
Timestamp: 2025-11-12T14:18:53.288Z
Learning: In packages/transition-backend/src/tasks/serverWorkerPool.ts, forced termination with pool.terminate(true) is preferred over graceful shutdown because when the parent thread is terminated (e.g., Ctrl-C), workers would continue running without access to necessary resources like the routing engine, leading to errors. It's better to forcefully kill tasks than let them run with missing dependencies.
Applied to files:
packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.tspackages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.ts
📚 Learning: 2025-09-25T17:09:26.332Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/tasks/TransitionWorkerPool.ts:85-91
Timestamp: 2025-09-25T17:09:26.332Z
Learning: In packages/transition-backend/src/tasks/TransitionWorkerPool.ts, greenscientist deferred improving input file validation to check actual file existence on disk (using getFilePath('input') instead of just hasInputFile() metadata check), indicating it should be addressed when file access is refactored soon.
Applied to files:
packages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.tspackages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.tspackages/transition-backend/src/services/transitRouting/BatchAccessibilityMapJob.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: When modifying transit data models that use Cap'n Proto caching, update the Cap'n Proto schema, regenerate TypeScript types, and regenerate the cache
Applied to files:
packages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.ts
📚 Learning: 2026-01-28T18:01:07.845Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1736
File: packages/transition-backend/src/services/generalizedCostFunction/GeneralizedCostFunction.ts:54-63
Timestamp: 2026-01-28T18:01:07.845Z
Learning: In packages/transition-backend/src/services/generalizedCostFunction/GeneralizedCostFunction.ts, the headway thresholds in getHeadwayThresholdSeconds() are intentionally hard-coded constants rather than configurable parameters. These values should be based on international consensus to allow consistent comparison of transit service quality across different cities. The TODO comments indicate that these values need more research and community discussion before being finalized.
Applied to files:
packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to packages/transition-frontend/src/components/routers/*.tsx : Application routing should be defined in `TransitionRouter.tsx` using React Router with feature-based route organization
Applied to files:
packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to **/*.{ts,tsx} : Document non-trivial functions and attributes using JSDoc or inline comments when short
Applied to files:
packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to packages/transition-backend/src/api/*.socketRoutes.ts : Socket.IO route handlers must be registered in `all.socketRoutes.ts` and organized by feature namespace
Applied to files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts
📚 Learning: 2025-11-18T19:37:07.564Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1588
File: packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.ts:11-23
Timestamp: 2025-11-18T19:37:07.564Z
Learning: In the chairemobilite/transition repository TypeScript code, greenscientist prefers that reviews highlight when interfaces could be replaced with type aliases for stylistic consistency, especially when the structure is just a data shape.
Applied to files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to **/*.ts : Use TypeScript strictly and avoid `any` types - maintain type safety throughout the application
Applied to files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts
📚 Learning: 2025-11-25T21:28:28.491Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1604
File: packages/transition-backend/src/services/weighting/NodeWeightCalculator.ts:74-79
Timestamp: 2025-11-25T21:28:28.491Z
Learning: In packages/transition-backend/src/services/weighting/NodeWeightCalculator.ts, the maxBirdDistanceMeters, maxNetworkDistanceMeters, and maxTravelTimeSeconds parameters should not accept 0 or negative values, as they are semantically invalid for distance and time thresholds. Explicit validation should be preferred over relying on falsy coercion.
Applied to files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts
🧬 Code graph analysis (4)
packages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.ts (1)
packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts (2)
MemcachedInstance(17-46)status(34-37)
packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.ts (1)
packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts (1)
MemcachedInstance(17-46)
packages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.ts (2)
packages/chaire-lib-backend/src/utils/filesystem/directoryManager.ts (1)
directoryManager(248-248)packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts (1)
status(34-37)
packages/transition-backend/src/services/transitRouting/BatchAccessibilityMapJob.ts (4)
packages/transition-common/src/services/transitDemand/types.ts (1)
TransitDemandFromCsvAccessMapAttributes(19-30)packages/transition-common/src/services/accessibilityMap/TransitBatchAccessibilityMap.ts (1)
TransitDemandFromCsvAccessMapAttributes(11-22)packages/transition-common/src/services/accessibilityMap/TransitAccessibilityMapRouting.ts (1)
AccessibilityMapAttributes(39-39)packages/transition-backend/src/services/transitRouting/TrRoutingBatchJobParameters.ts (1)
TrRoutingBatchJobParameters(12-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: code-lint
- GitHub Check: build-and-run-UI-tests (24.x)
- GitHub Check: build-and-test (24.x)
- GitHub Check: pr-build-check
- GitHub Check: test-sequential (24.x)
🔇 Additional comments (14)
packages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.ts (1)
24-27: LGTM — clean type addition and memcached lifecycle removal.
TrRoutingBatchStartParametersis appropriately scoped (non-exported) and thememcachedServerpropagation through tostartTrRoutingProcessis correct.packages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.ts (1)
51-58: LGTM — mock setup looks correct.The
MemcachedProcessManagermock is properly wired to providegetServer()soTrRoutingBatchManagercan propagate the server URL.packages/transition-backend/src/services/transitRouting/BatchAccessibilityMapJob.ts (1)
10-19: LGTM — clean extension of the job parameter type.packages/transition-backend/src/services/transitRouting/TrRoutingBatchJobParameters.ts (1)
22-34: LGTM — well-documented field addition.packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts (1)
55-83: No action needed. The emptywaitStringis intentional—ProcessManager treats it as a valid pattern where the process is considered started on spawn, rather than waiting for specific output. This is documented in the code and tested.packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts (4)
1-62: Well-structured test setup — looks good!Mocks are properly scoped and cleared in
beforeEach. The mock forMemcachedInstancecovers the full public interface. Nice work.
65-76: Good coverage of the self-managed memcached path.Verifies that
MemcachedProcessManager.startis called and the resulting server string is forwarded toTrRoutingProcessManager.startBatch.
172-184: Nice — graceful degradation when memcached start returns null.Good edge case coverage. TrRouting still starts, just without a memcached server.
188-196: Verify the expectedstopBatchcall count.The comment on line 194 says
stopBatchis called twice — once instartBatchcleanup and once instopBatch. This is correct per the implementation (line 87 ofTrRoutingBatchManager.tscallsstopBatch()before starting). Clear and well-documented assertion.packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.ts (4)
9-11: New imports look correct.Clean import of both the default export (
MemcachedProcessManager) and the named class (MemcachedInstance).
44-62: Constructor and new fields — clean design.The inline options type is appropriate here for a simple data shape. The
ownsMemcachedflag is a clear way to track lifecycle responsibility. Based on learnings, greenscientist prefers type aliases over interfaces for simple data shapes, and this inline approach is even lighter.
129-145:stopBatchmemcached cleanup looks solid.The ownership check (
this.ownsMemcached && this.memcachedInstance) correctly ensures only self-started instances are stopped, and state is properly reset. Thefinallyblock guarantees progress emission even if stop fails.
89-100: Consider the lifecycle design of this manager: is it intended for single use or reuse?The current usage pattern (in both
TrRoutingBatchandTrAccessibilityMapBatch) creates a freshTrRoutingBatchManagerfor each batch operation, callingstartBatch()once and cleanly stopping it in a finally block. No caller reuses the same manager instance, so the multiple-startBatch()scenario you described doesn't occur in practice.If this class is intentionally single-use (as the current usage suggests), no fix is needed. If it's intended to support reuse, your proposed fix to stop the previous instance before starting a new one is appropriate and defensively sound.
packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts (1)
57-60: Clean pass-through of routing job parameters — looks good.The optional
trRoutingJobParametersis properly forwarded tostartBatch, which accepts it as an optional parameter. Type alignment is correct and will work as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.ts`:
- Around line 129-143: The test is missing the 'error' branch for stop()—update
the test.each in MemcachedProcessManager.test.ts to include an entry for {
mockStatus: 'error', expectedStatus: 'error' } so the stopProcessMock resolution
covers the error branch and the assertion on result.status (and the call
expectation for stopProcessMock with 'memcached', 'memcached') verifies correct
behavior for instance.stop(); keep the same mockResolvedValue shape and
expectations as the other cases.
- Around line 55-60: The test contains a dead else branch because every test
case sets expectedInstance = true; update the test in
MemcachedProcessManager.test.ts by either adding a case where expectedInstance
is false to exercise the else branch or, more simply, remove the conditional
branch and assert only the positive expectations: replace the if/else around
expectedInstance with a single assertion that instance is not null
(expect(instance).not.toBeNull()) and then validate instance?.getServer() equals
expectedServer (expect(instance?.getServer()).toBe(expectedServer)); refer to
the test variables expectedInstance, instance, and expectedServer to locate the
code to change.
In
`@packages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.ts`:
- Line 652: Rename the test case name string in TrRoutingProcessManager.test.ts
from "start batch process with just memached parameters set" to "start batch
process with just memcached parameters set"; locate the Jest test declaration
with that exact title (the test(...) call) and update the test description only
(leave the test body and identifiers unchanged) to correct the typo.
- Around line 627-628: Replace double-quoted string literals with the project's
single-quote convention for the two constants: change the values assigned to
cacheDirectoryPath and memcachedServer to use single quotes ('/tmp/cache' and
'localhost:11212') so the declarations const cacheDirectoryPath and const
memcachedServer match the file's quote style.
In
`@packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.ts`:
- Around line 35-39: Remove the dead variable by either deleting the unused
mockMemcachedInstance declaration or wire it into the existing jest.mock factory
so the factory returns mockMemcachedInstance; locate the jest.mock call that
creates the inline memcached mock (the factory around lines where the test file
defines the mock) and change it to return the existing mockMemcachedInstance or
simply remove the standalone mockMemcachedInstance constant if you prefer the
inline factory object.
In
`@packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts`:
- Around line 142-154: The test uses an untyped mock return
(mockTrRoutingStartBatch.mockResolvedValue(... as any)); replace the use of "as
any" by returning a correctly typed mock object matching the real
TrRoutingProcessManager.startBatch response (or define a small interface/type
for the error response) and use that type when calling mockResolvedValue, or
annotate the literal with "as const" for each property; update the test's mocked
return so it matches the expected shape (status, service, port) with proper
typing and keep the rest of the assertion unchanged in
TrRoutingBatchManager.startBatch test.
---
Outside diff comments:
In
`@packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.ts`:
- Around line 80-121: In startBatch: if TrRoutingProcessManager.startBatch(...)
throws after we started a memcached instance via
MemcachedProcessManager.start(), ensure we stop that owned instance before
rethrowing; wrap the call to TrRoutingProcessManager.startBatch in a try/catch
(or add a nested try/finally) and when an error occurs check this.ownsMemcached
and this.memcachedInstance and call the appropriate cleanup (e.g.,
MemcachedProcessManager.stop(this.memcachedInstance) or
this.memcachedInstance.stop()) to stop the memcached instance, clear
this.memcachedInstance/this.ownsMemcached, then rethrow the original error.
---
Duplicate comments:
In
`@packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts`:
- Around line 17-21: The constructor in class MemcachedInstance currently uses
parameter-property syntax to set private port and includes a "/* Nothing to do
*/" comment to satisfy the linter; no code change is required—leave the
constructor as-is (constructor(private port: number) { /* Nothing to do */ }) to
keep the parameter assignment and lint suppression intact.
In
`@packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts`:
- Around line 124-140: Replace the three separate assertions with a
parameterized test using test.each to iterate the workload/expected pairs;
specifically update the test that instantiates TrRoutingBatchManager and calls
manager.startBatch(...) to use test.each([...]) with rows like [3,1], [9,3],
[100,4] and inside the row body call manager.startBatch(workload) and assert
mockTrRoutingStartBatch was last called with expectedThreads and
expect.anything(); keep the same setup (progressEmitter, memcachedServer) and
the reference to TrRoutingBatchManager.startBatch and mockTrRoutingStartBatch so
behavior and expectations are unchanged.
- Around line 156-158: The test declares progressEvents as any[]; replace this
with the concrete event type emitted by progressEmitter (e.g., ProgressEvent,
ProgressPayload, or Array<YourProgressType>) by importing or referencing the
existing interface used by the emitter (the same type used where progress events
are defined/typed in the transit routing code), and update the second occurrence
of progressEvents elsewhere in the file to use the same concrete type so the
test has strong typing and catches shape errors at compile time.
packages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.ts
Outdated
Show resolved
Hide resolved
packages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.ts
Show resolved
Hide resolved
packages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.ts
Outdated
Show resolved
Hide resolved
packages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.ts
Outdated
Show resolved
Hide resolved
| const mockMemcachedInstance = { | ||
| getServer: jest.fn().mockReturnValue('localhost:11212'), | ||
| status: jest.fn().mockResolvedValue('running'), | ||
| stop: jest.fn().mockResolvedValue({ status: 'stopped' }) | ||
| }; |
There was a problem hiding this comment.
mockMemcachedInstance is declared but never used — dead code.
The jest.mock factory at lines 41–47 creates its own inline object, making mockMemcachedInstance unreferenced.
🛠️ Fix
-// Mock MemcachedProcessManager
-const mockMemcachedInstance = {
- getServer: jest.fn().mockReturnValue('localhost:11212'),
- status: jest.fn().mockResolvedValue('running'),
- stop: jest.fn().mockResolvedValue({ status: 'stopped' })
-};
-
jest.mock('chaire-lib-backend/lib/utils/processManagers/MemcachedProcessManager', () => ({📝 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.
| const mockMemcachedInstance = { | |
| getServer: jest.fn().mockReturnValue('localhost:11212'), | |
| status: jest.fn().mockResolvedValue('running'), | |
| stop: jest.fn().mockResolvedValue({ status: 'stopped' }) | |
| }; | |
| jest.mock('chaire-lib-backend/lib/utils/processManagers/MemcachedProcessManager', () => ({ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.ts`
around lines 35 - 39, Remove the dead variable by either deleting the unused
mockMemcachedInstance declaration or wire it into the existing jest.mock factory
so the factory returns mockMemcachedInstance; locate the jest.mock call that
creates the inline memcached mock (the factory around lines where the test file
defines the mock) and change it to return the existing mockMemcachedInstance or
simply remove the standalone mockMemcachedInstance constant if you prefer the
inline factory object.
| test('should throw error when TrRouting fails to start', async () => { | ||
| mockTrRoutingStartBatch.mockResolvedValue({ | ||
| status: 'error', | ||
| service: 'trRoutingBatch', | ||
| port: 14000 | ||
| } as any); | ||
|
|
||
| const manager = new TrRoutingBatchManager(progressEmitter, { | ||
| memcachedServer: 'external:11211' | ||
| }); | ||
|
|
||
| await expect(manager.startBatch(10)).rejects.toThrow('Failed to start TrRouting instance: error'); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the TrRoutingProcessManager file and check the startBatch method
find . -name "TrRoutingProcessManager.ts" -type f | head -20Repository: chairemobilite/transition
Length of output: 151
🏁 Script executed:
# Search for startBatch method definition in the codebase
rg -n "startBatch" --type ts -g "!*test*" -g "!*__tests__*" -A 5 | head -40Repository: chairemobilite/transition
Length of output: 4494
🏁 Script executed:
# Look for ProcessStartStatus or similar type definitions
rg -n "ProcessStartStatus\|interface.*Start.*Status\|type.*Start.*Status" --type ts | head -40Repository: chairemobilite/transition
Length of output: 51
🏁 Script executed:
# Read the startBatch function and its return type from TrRoutingProcessManager
sed -n '191,230p' ./packages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.tsRepository: chairemobilite/transition
Length of output: 1428
🏁 Script executed:
# Also check the TrRoutingBatchManager.startBatch return type
sed -n '80,120p' ./packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.tsRepository: chairemobilite/transition
Length of output: 1890
🏁 Script executed:
# Look for TrRoutingBatchStartResult type definition
rg -n "TrRoutingBatchStartResult" --type ts -A 10 | head -50Repository: chairemobilite/transition
Length of output: 2762
🏁 Script executed:
# Search for Status/Result pattern from chaire-lib-common
rg -n "Status.*Result\|Result.*Status" --type ts packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.ts | head -20Repository: chairemobilite/transition
Length of output: 51
🏁 Script executed:
# Check what types are imported in the test file
head -30 packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tsRepository: chairemobilite/transition
Length of output: 1197
🏁 Script executed:
# Look at how TrRoutingProcessManager.startBatch is defined - does it have an explicit return type?
rg -B 5 "const startBatch = async function" packages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.tsRepository: chairemobilite/transition
Length of output: 155
🏁 Script executed:
# Look for the full signature of TrRoutingProcessManager.startBatch with return type
sed -n '185,235p' ./packages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.tsRepository: chairemobilite/transition
Length of output: 1684
🏁 Script executed:
# Check if there's a type definition for the return value
rg -n "startBatch.*:" packages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.tsRepository: chairemobilite/transition
Length of output: 51
Avoid as any — use a typed mock return value.
The test intentionally mocks an error scenario, but the actual TrRoutingProcessManager.startBatch returns only { status: 'started', ... }. Using as const on each property is better than as any, but consider defining a proper type for the mock that reflects the test's intent:
♻️ Suggested approach
+ const errorStartResult = {
+ status: 'error',
+ service: 'trRoutingBatch',
+ port: 14000
+ };
- mockTrRoutingStartBatch.mockResolvedValue({
- status: 'error',
- service: 'trRoutingBatch',
- port: 14000
- } as any);
+ mockTrRoutingStartBatch.mockResolvedValue(errorStartResult);This avoids any and makes the mock value explicit. Alternatively, use as const on each property if you prefer inline.
📝 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.
| test('should throw error when TrRouting fails to start', async () => { | |
| mockTrRoutingStartBatch.mockResolvedValue({ | |
| status: 'error', | |
| service: 'trRoutingBatch', | |
| port: 14000 | |
| } as any); | |
| const manager = new TrRoutingBatchManager(progressEmitter, { | |
| memcachedServer: 'external:11211' | |
| }); | |
| await expect(manager.startBatch(10)).rejects.toThrow('Failed to start TrRouting instance: error'); | |
| }); | |
| test('should throw error when TrRouting fails to start', async () => { | |
| const errorStartResult = { | |
| status: 'error', | |
| service: 'trRoutingBatch', | |
| port: 14000 | |
| }; | |
| mockTrRoutingStartBatch.mockResolvedValue(errorStartResult); | |
| const manager = new TrRoutingBatchManager(progressEmitter, { | |
| memcachedServer: 'external:11211' | |
| }); | |
| await expect(manager.startBatch(10)).rejects.toThrow('Failed to start TrRouting instance: error'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts`
around lines 142 - 154, The test uses an untyped mock return
(mockTrRoutingStartBatch.mockResolvedValue(... as any)); replace the use of "as
any" by returning a correctly typed mock object matching the real
TrRoutingProcessManager.startBatch response (or define a small interface/type
for the error response) and use that type when calling mockResolvedValue, or
annotate the literal with "as const" for each property; update the test's mocked
return so it matches the expected shape (status, service, port) with proper
typing and keep the rest of the assertion unchanged in
TrRoutingBatchManager.startBatch test.
tahini
left a comment
tahini
left a comment
There was a problem hiding this comment.
Quelques considératiosn qui requiert peut-être au moins des FIXME ou documentation.
| * @returns 'running' if the process is running, 'not_running' otherwise | ||
| */ | ||
| async status(): Promise<'running' | 'not_running'> { | ||
| const isRunning = await ProcessManager.isServiceRunning(SERVICE_NAME); |
There was a problem hiding this comment.
Il ne faudrait pas lui ajouter le numéro de port ici? Tout d'un coup que 2 instances soit démarrées sur 2 ports différents, qui tuerait l'instance de l'autre port avant de démarrer celle sur le nouveau et rendrait le trRouting qui l'utilise inutilisable? Sinon, il faudrait mentionner dans la doc de la classe que Transition ne roule qu'une seule instance de memcached, peu importe le port...
There was a problem hiding this comment.
Oui, j'ai ce changement en local, ca va être là au prochain push.
packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts
Outdated
Show resolved
Hide resolved
| }; | ||
|
|
||
| jest.mock('chaire-lib-backend/lib/utils/processManagers/MemcachedProcessManager', () => ({ | ||
| start: jest.fn().mockResolvedValue({ |
There was a problem hiding this comment.
Pour éviter d'avoir à réécrire la même chose que le mockMemcachedInstance et éviter la variable non-définie au moment d'écrire le mock, tu peux faire un mockImplementation(() => mockMemcachedInstance) à la place, la variable sera définie au moment de l'appel.
| * | ||
| * Memcached is handled as follows: | ||
| * 1. If options.memcachedServer is provided, use it | ||
| * 2. Else if constructor options.memcachedServer was provided, use it |
There was a problem hiding this comment.
Please try to remember and have a good explanation for why the options here. The way I see it, if you call it with options, after having called it without options, for example, it may override the object's memcachedInstance and ownsMemcached and you may have a rogue memcached instance lost somewhere. You should not be able to call the startBatch function more than once with different options. Plus with the service name without a port, you may end up with an instance running on a different port than the one you want.
| if (!memcachedServer) { | ||
| // Start our own memcached instance | ||
| const instance = await MemcachedProcessManager.start(); | ||
| if (instance) { |
There was a problem hiding this comment.
Here if the memcached was already_running, should you not set yourself as owner? There might be 2 owners of the process and one may stop it before the other is done with it (but then the problem would be the same if you do not own it and the owner stops it before this one is done)
There was a problem hiding this comment.
yeah good point, did not think of that angle.
We create a new MemcachedProcessManager, that will return a MemcachedInstance object, when we start it. We can then get the status, serverUrl and stop it from this object. TrRoutingBatchManager is now responsible to start and stop memcached, unless the jobs have received a memcachedServer string to use when starting trRouting. This will allow to start a memcached and have several jobs use the same one. We are planning to use this in the Genetic algorithm simulations
greenscientist
force-pushed
the
memcachedlonglife
branch
from
651e212 to
e9a3cf1
Compare
|
(still some work to do, push this version to close some of the comments) |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts (1)
41-43: 🧹 Nitpick | 🔵 TrivialReminder: deferred input file validation is still open.
The
if (!job.hasInputFile())guard was deferred in a previous PR — worth tracking for the upcoming file-access refactor. Based on learnings from greenscientist (PR 1494): input file validation usingif (!job.hasInputFile())was explicitly deferred and requested to be raised in a later PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts` around lines 41 - 43, The code currently calls job.getInputFileName() without guarding for missing input as the earlier deferred check (job.hasInputFile()) was postponed; restore a defensive check in TrAccessibilityMapBatch before calling getInputFileName(): verify job.hasInputFile() and if false throw or return a clear error, otherwise call job.getInputFileName() to populate files.input; reference the job.hasInputFile() and job.getInputFileName() calls so the guard is added right where files is created to prevent uncaught exceptions during the upcoming file-access refactor.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.tspackages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.tspackages/transition-backend/src/services/transitRouting/BatchAccessibilityMapJob.tspackages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.tspackages/transition-backend/src/services/transitRouting/TrRoutingBatchJobParameters.tspackages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.tspackages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Use TypeScript strictly and avoid
anytypes - maintain type safety throughout the application
Files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tspackages/transition-backend/src/services/transitRouting/TrRoutingBatchJobParameters.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.tspackages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.tspackages/transition-backend/src/services/transitRouting/BatchAccessibilityMapJob.tspackages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.tspackages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.tspackages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.tspackages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
**/*.{ts,tsx}: Use UUIDs as primary keys for most entities in the database
Use the Status/Result pattern from chaire-lib-common for service functions and handlers
Indent using 4 spaces
Do not use trailing spaces, even in comments or documentation; replace lines with only spaces with blank lines
Use parentheses for arrow function parameters
Document non-trivial functions and attributes using JSDoc or inline comments when short
Use Geographic data in GeoJSON format with coordinates as [longitude, latitude]
Use i18next and thet()function for all user-facing strings to support internationalization
Files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tspackages/transition-backend/src/services/transitRouting/TrRoutingBatchJobParameters.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.tspackages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.tspackages/transition-backend/src/services/transitRouting/BatchAccessibilityMapJob.tspackages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.tspackages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.tspackages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.tspackages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests
Files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.ts
🧠 Learnings (17)
📚 Learning: 2025-09-25T16:09:00.577Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts:42-45
Timestamp: 2025-09-25T16:09:00.577Z
Learning: In packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts, greenscientist deferred adding input file validation using `if (!job.hasInputFile())` and requested to be reminded about it in a later PR.
Applied to files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tspackages/transition-backend/src/services/transitRouting/TrRoutingBatchJobParameters.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.tspackages/transition-backend/src/services/transitRouting/BatchAccessibilityMapJob.tspackages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.tspackages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.tspackages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.tspackages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.ts
📚 Learning: 2025-09-25T17:08:56.828Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/tasks/TransitionWorkerPool.ts:105-107
Timestamp: 2025-09-25T17:08:56.828Z
Learning: In packages/transition-backend/src/tasks/TransitionWorkerPool.ts, greenscientist deferred changing the wrapBatchAccessMap input file validation from `hasInputFile()` to `getFilePath('input')` (actual file existence check), indicating it will be addressed in an upcoming file access refactor.
Applied to files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.tspackages/transition-backend/src/services/transitRouting/BatchAccessibilityMapJob.tspackages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.tspackages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.tspackages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.ts
📚 Learning: 2025-10-20T18:31:07.612Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1516
File: packages/chaire-lib-backend/src/utils/trRouting/TrRoutingServiceBackend.ts:29-41
Timestamp: 2025-10-20T18:31:07.612Z
Learning: In TrRoutingServiceBackend (packages/chaire-lib-backend/src/utils/trRouting/TrRoutingServiceBackend.ts), the fetch-retry package was not working as expected, so custom retry logic was implemented instead.
Applied to files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.tspackages/transition-backend/src/services/transitRouting/BatchAccessibilityMapJob.tspackages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.tspackages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.tspackages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.tspackages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.ts
📚 Learning: 2025-09-25T17:09:26.332Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/tasks/TransitionWorkerPool.ts:85-91
Timestamp: 2025-09-25T17:09:26.332Z
Learning: In packages/transition-backend/src/tasks/TransitionWorkerPool.ts, greenscientist deferred improving input file validation to check actual file existence on disk (using getFilePath('input') instead of just hasInputFile() metadata check), indicating it should be addressed when file access is refactored soon.
Applied to files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tspackages/transition-backend/src/services/transitRouting/BatchAccessibilityMapJob.tspackages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.tspackages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to packages/transition-backend/src/api/*.socketRoutes.ts : Socket.IO route handlers must be registered in `all.socketRoutes.ts` and organized by feature namespace
Applied to files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.ts
📚 Learning: 2025-09-25T16:40:55.314Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/services/executableJob/ExecutableJob.ts:152-158
Timestamp: 2025-09-25T16:40:55.314Z
Learning: In packages/transition-backend/src/services/executableJob/ExecutableJob.ts, greenscientist deferred improving the hasFile() method to check actual file existence on disk rather than just metadata, indicating it should be addressed in a later PR.
Applied to files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to packages/transition-frontend/src/components/routers/*.tsx : Application routing should be defined in `TransitionRouter.tsx` using React Router with feature-based route organization
Applied to files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to **/*.test.{ts,tsx} : Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests
Applied to files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.tspackages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.tspackages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.ts
📚 Learning: 2025-11-04T19:35:10.045Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1554
File: packages/transition-common/src/services/simulation/simulationMethod/OdTripSimulationMethod.ts:23-36
Timestamp: 2025-11-04T19:35:10.045Z
Learning: In the OdTripSimulationMethod (packages/transition-common/src/services/simulation/simulationMethod/OdTripSimulationMethod.ts), the team plans to move away from the datasource path and use batch calculations instead. The current non-functional dataSourceId selector will be reworked as part of this architectural change.
Applied to files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.tspackages/transition-backend/src/services/transitRouting/BatchAccessibilityMapJob.tspackages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.tspackages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts
📚 Learning: 2025-11-18T19:37:07.564Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1588
File: packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.ts:11-23
Timestamp: 2025-11-18T19:37:07.564Z
Learning: In the chairemobilite/transition repository TypeScript code, greenscientist prefers that reviews highlight when interfaces could be replaced with type aliases for stylistic consistency, especially when the structure is just a data shape.
Applied to files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to **/*.ts : Use TypeScript strictly and avoid `any` types - maintain type safety throughout the application
Applied to files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts
📚 Learning: 2025-11-25T21:28:28.491Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1604
File: packages/transition-backend/src/services/weighting/NodeWeightCalculator.ts:74-79
Timestamp: 2025-11-25T21:28:28.491Z
Learning: In packages/transition-backend/src/services/weighting/NodeWeightCalculator.ts, the maxBirdDistanceMeters, maxNetworkDistanceMeters, and maxTravelTimeSeconds parameters should not accept 0 or negative values, as they are semantically invalid for distance and time thresholds. Explicit validation should be preferred over relying on falsy coercion.
Applied to files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to **/*.{ts,tsx} : Use the Status/Result pattern from chaire-lib-common for service functions and handlers
Applied to files:
packages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to **/*.{ts,tsx} : Document non-trivial functions and attributes using JSDoc or inline comments when short
Applied to files:
packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: When modifying transit data models that use Cap'n Proto caching, update the Cap'n Proto schema, regenerate TypeScript types, and regenerate the cache
Applied to files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.ts
📚 Learning: 2025-12-17T22:46:52.336Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1666
File: packages/chaire-lib-frontend/package.json:60-60
Timestamp: 2025-12-17T22:46:52.336Z
Learning: In packages/chaire-lib-frontend/package.json, the `testing-library/dom` devDependency is required for tests to pass even though it's not explicitly imported. It was previously provided as a transitive dependency through `react-select-event`, but after removing `react-select-event`, it must be explicitly declared.
Applied to files:
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.ts
📚 Learning: 2025-11-12T14:18:53.288Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1568
File: packages/transition-backend/src/tasks/serverWorkerPool.ts:25-31
Timestamp: 2025-11-12T14:18:53.288Z
Learning: In packages/transition-backend/src/tasks/serverWorkerPool.ts, forced termination with pool.terminate(true) is preferred over graceful shutdown because when the parent thread is terminated (e.g., Ctrl-C), workers would continue running without access to necessary resources like the routing engine, leading to errors. It's better to forcefully kill tasks than let them run with missing dependencies.
Applied to files:
packages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.tspackages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.ts
🧬 Code graph analysis (4)
packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts (2)
packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts (1)
MemcachedInstance(20-49)packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.ts (1)
TrRoutingBatchManager(44-161)
packages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.ts (1)
packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts (2)
MemcachedInstance(20-49)status(37-40)
packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.ts (1)
packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts (1)
MemcachedInstance(20-49)
packages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.ts (2)
packages/chaire-lib-backend/src/utils/filesystem/directoryManager.ts (1)
directoryManager(248-248)packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts (1)
status(37-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-and-test (24.x)
- GitHub Check: test-sequential (24.x)
- GitHub Check: build-and-run-UI-tests (24.x)
- GitHub Check: pr-build-check
🔇 Additional comments (17)
packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts (2)
1-14: Clean module setup and good documentation throughout. The constants, helper function, and JSDoc comments are well-organized.
20-49: MemcachedInstance class looks good. Methods are concise and delegate properly to ProcessManager. The constructor comment addressing the lint concern is appropriate.packages/chaire-lib-backend/src/utils/processManagers/__tests__/MemcachedProcessManager.test.ts (2)
24-91: Good use of parametric tests for both success and error paths. Thetest.eachstructure makes it easy to add new cases.
94-137: MemcachedInstance tests look thorough. All three stop statuses are covered, and status tests use parametric approach nicely.packages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.ts (3)
24-26: Clean type extension for batch parameters. The intersection type with optionalmemcachedServeris straightforward.
191-229:startBatchrefactoring is clean. Memcached lifecycle is properly removed, andmemcachedServerflows through tostartTrRoutingProcessvia the params object. Nice simplification.
231-239:stopBatchis now leaner. Memcached stop responsibility is correctly moved to the caller.packages/transition-backend/src/services/transitRouting/TrRoutingBatchJobParameters.ts (1)
22-34: Excellent documentation. The JSDoc clearly explains both scenarios (external vs. self-managed memcached). Well done.packages/chaire-lib-backend/src/utils/processManagers/__tests__/TrRoutingProcessManager.test.ts (2)
501-523: Good:startProcessMockcall count reduced to 1. Correctly reflects that memcached is no longer started insidestartBatch.
624-675: New memcached tests are solid. Both "all parameters" and "just memcached" scenarios are well covered. The--useMemcached=localhost:11212argument is correctly asserted.packages/transition-backend/src/services/transitRouting/__tests__/TrAccessibilityMapBatch.test.ts (1)
51-61: MemcachedProcessManager mock is correctly set up. The mock provides the expected interface (getServer, status, stop) to prevent real process spawning during tests.packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.ts (1)
231-237: Test correctly verifies memcachedServer propagation. The assertion thatstartBatchreceivesmemcachedServer: 'localhost:11212'validates the new integration path.packages/transition-backend/src/services/transitRouting/BatchAccessibilityMapJob.ts (1)
10-18: Clean type extension. Adding the optionaltrRoutingJobParametersfield is a nice way to propagate routing configuration without breaking existing job payloads.packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts (1)
57-60: LGTM! Cleanly threadstrRoutingJobParametersintostartBatch.packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts (1)
65-122: LGTM! Good coverage of all threememcachedServerprecedence paths.packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.ts (2)
44-62: LGTM! Clean constructor with correct optional typing.
129-145: LGTM! Correct teardown order and ownership cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/chaire-lib-backend/src/utils/processManagers/MemcachedProcessManager.ts`:
- Around line 75-88: The start logic in MemcachedProcessManager (when
processStatus.status === 'already_running') currently logs an error and returns
null, causing callers to treat an existing memcached as failure; change this
behavior to return a MemcachedInstance for the given port (new
MemcachedInstance(port)) so callers can use the already-running process, or if
double-ownership is intentional, add a clear inline comment in
MemcachedProcessManager.ts explaining the rationale; update the branch that
checks processStatus.status === 'already_running' to either return new
MemcachedInstance(port) or contain the explanatory comment so future developers
understand the decision.
In
`@packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.ts`:
- Around line 89-100: The startBatch logic creates a local memcachedServer
variable each call which allows a second call to re-enter the "if
(!memcachedServer)" branch and start (and overwrite) a new MemcachedProcess,
orphaning the prior process; update the startBatch flow to first consult
instance state (this.memcachedInstance and this.ownsMemcached) before starting a
new one: if this.memcachedInstance exists use its getServer(), only call
MemcachedProcessManager.start() when there is truly no existing instance and set
this.memcachedInstance, this.ownsMemcached and memcachedServer from that single
source so repeated startBatch invocations reuse the existing instance instead of
leaking processes (refer to memcachedServer local, this.memcachedInstance,
this.ownsMemcached and MemcachedProcessManager.start()).
---
Outside diff comments:
In
`@packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts`:
- Around line 41-43: The code currently calls job.getInputFileName() without
guarding for missing input as the earlier deferred check (job.hasInputFile())
was postponed; restore a defensive check in TrAccessibilityMapBatch before
calling getInputFileName(): verify job.hasInputFile() and if false throw or
return a clear error, otherwise call job.getInputFileName() to populate
files.input; reference the job.hasInputFile() and job.getInputFileName() calls
so the guard is added right where files is created to prevent uncaught
exceptions during the upcoming file-access refactor.
---
Duplicate comments:
In
`@packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.ts`:
- Around line 34-48: The declared mockMemcachedInstance object is unused because
the jest.mock factory returns its own inline object; either remove the unused
mockMemcachedInstance declaration or update the mock factory for
MemcachedProcessManager to return mockMemcachedInstance so the test reuses that
single mock instance (adjust the jest.mock factory around
MemcachedProcessManager.start and the mock assignment to mockMemcachedStart to
reference mockMemcachedInstance).
In
`@packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatchManager.test.ts`:
- Around line 124-140: Replace the single test containing three sequential
expects with a parametric test using test.each: build an array of cases like
[{workload:3, expected:1}, {workload:9, expected:3}, {workload:100,
expected:4}], then for each row instantiate TrRoutingBatchManager (using
progressEmitter and the memcachedServer option), await
manager.startBatch(workload) and assert mockTrRoutingStartBatch was last called
with expected and expect.anything(); reference the TrRoutingBatchManager
constructor, startBatch method and the mockTrRoutingStartBatch mock in the
table-driven test so each workload is its own failing case.
- Around line 156-158: The progressEvents array is typed as any[]; change its
type to Array<{ name: string; progress: number }> in the test 'should emit
progress events' (variable progressEvents) and the other occurrence (line with
the second progressEvents declaration) so both declarations use a concrete type
instead of any; leave the progressEmitter.on('progress', ...) listener as-is so
it pushes objects conforming to { name: string; progress: number } into
progressEvents.
- Around line 142-154: Replace the ad-hoc "as any" on the mocked resolved value
for mockTrRoutingStartBatch with a properly typed mock object matching the real
start result type; update the test to type the mock value (e.g., const
mockResult: ReturnType<typeof trRouting.startBatch> = { status: 'error',
service: 'trRoutingBatch', port: 14000 } or import the exported
TrRoutingStartResult/StartBatchResponse type and use that) and pass mockResult
to mockResolvedValue instead of using "as any", referencing
mockTrRoutingStartBatch and the TrRoutingBatchManager.startBatch test setup.
| if (processStatus.status === 'started') { | ||
| return new MemcachedInstance(port); | ||
| } | ||
|
|
||
| if (processStatus.status === 'already_running') { | ||
| 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; | ||
| }; |
There was a problem hiding this comment.
already_running returns null — is that intentional?
If memcached is already running on the requested port (e.g. from a previous run that wasn't cleaned up), start returns null, which callers will interpret as a failure. Consider returning a MemcachedInstance for the existing process so the caller can still use it, or at least document that callers should handle this case.
If this is intentional (e.g. to prevent double-ownership of the same process), a brief inline comment explaining the rationale would help future readers.
🤖 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 75 - 88, The start logic in MemcachedProcessManager (when
processStatus.status === 'already_running') currently logs an error and returns
null, causing callers to treat an existing memcached as failure; change this
behavior to return a MemcachedInstance for the given port (new
MemcachedInstance(port)) so callers can use the already-running process, or if
double-ownership is intentional, add a clear inline comment in
MemcachedProcessManager.ts explaining the rationale; update the branch that
checks processStatus.status === 'already_running' to either return new
MemcachedInstance(port) or contain the explanatory comment so future developers
understand the decision.
There was a problem hiding this comment.
yeah, sorry, I wanted to add a comment but forgot!
There was a problem hiding this comment.
@greenscientist No worries! If you'd like, I can suggest wording for the comment, or feel free to add whatever works best for your use case. Happy to help if needed. 😊
| // Determine memcached server: options param > constructor options > start new one | ||
| let memcachedServer = options?.memcachedServer ?? this.options?.memcachedServer; | ||
|
|
||
| if (!memcachedServer) { | ||
| // Start our own memcached instance | ||
| const instance = await MemcachedProcessManager.start(); | ||
| if (instance) { | ||
| this.memcachedInstance = instance; | ||
| memcachedServer = instance.getServer(); | ||
| this.ownsMemcached = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Orphaned memcached process if startBatch is called again without stopBatch.
memcachedServer is a fresh local variable on every call. On a second invocation (when ownsMemcached = true), it resolves to undefined again, enters the if (!memcachedServer) block, starts a new instance, and overwrites this.memcachedInstance — silently leaking the previous process. Given the PR's goal of enabling instance reuse for GA simulations, this defensive guard is worth adding.
🛡️ Proposed fix
if (!memcachedServer) {
+ // Stop existing owned instance before starting a new one
+ if (this.ownsMemcached && this.memcachedInstance) {
+ await this.memcachedInstance.stop();
+ this.memcachedInstance = undefined;
+ this.ownsMemcached = false;
+ }
// Start our own memcached instance
const instance = await MemcachedProcessManager.start();
if (instance) {📝 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.
| // Determine memcached server: options param > constructor options > start new one | |
| let memcachedServer = options?.memcachedServer ?? this.options?.memcachedServer; | |
| if (!memcachedServer) { | |
| // Start our own memcached instance | |
| const instance = await MemcachedProcessManager.start(); | |
| if (instance) { | |
| this.memcachedInstance = instance; | |
| memcachedServer = instance.getServer(); | |
| this.ownsMemcached = true; | |
| } | |
| } | |
| // Determine memcached server: options param > constructor options > start new one | |
| let memcachedServer = options?.memcachedServer ?? this.options?.memcachedServer; | |
| if (!memcachedServer) { | |
| // Stop existing owned instance before starting a new one | |
| if (this.ownsMemcached && this.memcachedInstance) { | |
| await this.memcachedInstance.stop(); | |
| this.memcachedInstance = undefined; | |
| this.ownsMemcached = false; | |
| } | |
| // Start our own memcached instance | |
| const instance = await MemcachedProcessManager.start(); | |
| if (instance) { | |
| this.memcachedInstance = instance; | |
| memcachedServer = instance.getServer(); | |
| this.ownsMemcached = true; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.ts`
around lines 89 - 100, The startBatch logic creates a local memcachedServer
variable each call which allows a second call to re-enter the "if
(!memcachedServer)" branch and start (and overwrite) a new MemcachedProcess,
orphaning the prior process; update the startBatch flow to first consult
instance state (this.memcachedInstance and this.ownsMemcached) before starting a
new one: if this.memcachedInstance exists use its getServer(), only call
MemcachedProcessManager.start() when there is truly no existing instance and set
this.memcachedInstance, this.ownsMemcached and memcachedServer from that single
source so repeated startBatch invocations reuse the existing instance instead of
leaking processes (refer to memcachedServer local, this.memcachedInstance,
this.ownsMemcached and MemcachedProcessManager.start()).
2 participants
We create a new MemcachedProcessManager, that will return a MemcachedInstance object, when we start it. We can then get the status, serverUrl and stop it from this object.
TrRoutingBatchManager is now responsible to start and stop memcached, unless the jobs have received a memcachedServer string to use when starting trRouting.
This will allow to start a memcached and have several jobs use the same one. We are planning to use this in the Genetic algorithm simulations
Summary by CodeRabbit