From a9c413857aefaa84fb1e14cbbfb2c80e806881c9 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Tue, 17 Mar 2026 12:46:30 +0100 Subject: [PATCH] [clsi] avoid destroying containers of recently accessed projects (#32186) * [clsi] avoid destroying containers of recently accessed projects Co-authored-by: Anna Claire Fields * [clsi] gracefully handle missing access time during container cleanup * [clsi] fix cyclic import --------- Co-authored-by: Anna Claire Fields GitOrigin-RevId: 8195b6fccbe26d2fd673d38356af5d44cf4042a3 --- services/clsi/app/js/LastProjectAccess.js | 12 ++++++ .../clsi/app/js/ProjectPersistenceManager.js | 5 +-- .../clsi/test/unit/js/DockerRunner.test.js | 40 +++++++++++++++++-- 3 files changed, 50 insertions(+), 7 deletions(-) create mode 100644 services/clsi/app/js/LastProjectAccess.js diff --git a/services/clsi/app/js/LastProjectAccess.js b/services/clsi/app/js/LastProjectAccess.js new file mode 100644 index 0000000000..cca52ac75d --- /dev/null +++ b/services/clsi/app/js/LastProjectAccess.js @@ -0,0 +1,12 @@ +/** + * LAST_ACCESS is "owned" by ProjectPersistenceManager, but needs to be accessed + * by the DockerRunner. Which in turn is imported through CompileManager from + * ProjectPersistenceManager. Avoid this cyclic import with an extra module. + */ + +// projectId -> timestamp mapping. +export const LAST_ACCESS = new Map() + +export function getLastProjectAccessTime(projectId) { + return LAST_ACCESS.get(projectId) ?? 0 +} diff --git a/services/clsi/app/js/ProjectPersistenceManager.js b/services/clsi/app/js/ProjectPersistenceManager.js index ef47b30b1b..be9bd05252 100644 --- a/services/clsi/app/js/ProjectPersistenceManager.js +++ b/services/clsi/app/js/ProjectPersistenceManager.js @@ -17,12 +17,11 @@ import { callbackify } from 'node:util' import Path from 'node:path' import fs from 'node:fs' import * as HistoryResourceWriter from './HistoryResourceWriter.js' +import { LAST_ACCESS } from './LastProjectAccess.js' + let ProjectPersistenceManager const oneDay = 24 * 60 * 60 * 1000 -// projectId -> timestamp mapping. -const LAST_ACCESS = new Map() - let ANY_DISK_LOW = false let ANY_DISK_CRITICAL_LOW = false diff --git a/services/clsi/test/unit/js/DockerRunner.test.js b/services/clsi/test/unit/js/DockerRunner.test.js index 1b1c33214c..b591cf93a3 100644 --- a/services/clsi/test/unit/js/DockerRunner.test.js +++ b/services/clsi/test/unit/js/DockerRunner.test.js @@ -57,6 +57,14 @@ describe('DockerRunner', () => { }, })) + ctx.LastProjectAccess = { + getLastProjectAccessTime: sinon + .stub() + .returns(Date.now() - 24 * 60 * 60 * 1000), + } + + vi.doMock('../../../app/js/LastProjectAccess', () => ctx.LastProjectAccess) + vi.doMock('../../../app/js/LockManager', () => ({ default: { runWithLock(key, runner, callback) { @@ -782,17 +790,26 @@ describe('DockerRunner', () => { const oneHourInSeconds = 60 * 60 const oneHourInMilliseconds = oneHourInSeconds * 1000 const nowInSeconds = Date.now() / 1000 + ctx.recentlyAccessProjectId = '68494e7daa5c3680ee7182b1' + ctx.LastProjectAccess.getLastProjectAccessTime + .withArgs(ctx.recentlyAccessProjectId) + .returns(Date.now() - 1) ctx.containers = [ { - Name: '/project-old-container-name', + Name: '/project-69b72c66b1a8c2f5846b24a8-container-name', Id: 'old-container-id', Created: nowInSeconds - oneHourInSeconds - 100, }, { - Name: '/project-new-container-name', + Name: '/project-69b72c66b1a8c2f5846b24a9-container-name', Id: 'new-container-id', Created: nowInSeconds - oneHourInSeconds + 100, }, + { + Name: `/project-${ctx.recentlyAccessProjectId}-container-name`, + Id: 'recent-access-container-id', + Created: nowInSeconds - 2 * oneHourInSeconds, + }, { Name: '/totally-not-a-project-container', Id: 'some-random-id', @@ -816,13 +833,28 @@ describe('DockerRunner', () => { it('should destroy old containers', ctx => { ctx.DockerRunner.destroyContainer.callCount.should.equal(1) ctx.DockerRunner.destroyContainer - .calledWith('project-old-container-name', 'old-container-id') + .calledWith( + 'project-69b72c66b1a8c2f5846b24a8-container-name', + 'old-container-id' + ) .should.equal(true) }) it('should not destroy new containers', ctx => { ctx.DockerRunner.destroyContainer - .calledWith('project-new-container-name', 'new-container-id') + .calledWith( + 'project-69b72c66b1a8c2f5846b24a9-container-name', + 'new-container-id' + ) + .should.equal(false) + }) + + it('should not destroy old containers that were recently used', ctx => { + ctx.DockerRunner.destroyContainer + .calledWith( + 'project-68494e7daa5c3680ee7182b1-container-name', + 'recent-access-container-id' + ) .should.equal(false) })