Skip to content

Commit d18ce36

Browse files
authored
fix cfnlint init timing issue (#177)
1 parent 86e958f commit d18ce36

File tree

2 files changed

+79
-0
lines changed

2 files changed

+79
-0
lines changed

src/services/cfnLint/CfnLintService.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ export class CfnLintService implements SettingsConfigurable, Closeable {
5151
private initializationPromise?: Promise<void>;
5252
private readonly workerManager: PyodideWorkerManager;
5353
private readonly log = LoggerFactory.getLogger(CfnLintService);
54+
private readonly mountedFolders = new Map<string, WorkspaceFolder>();
5455

5556
@Telemetry() private readonly telemetry!: ScopedTelemetry;
5657

@@ -123,6 +124,19 @@ export class CfnLintService implements SettingsConfigurable, Closeable {
123124
try {
124125
// Initialize the worker manager
125126
await this.workerManager.initialize();
127+
128+
// Remount previously mounted folders after worker recovery
129+
if (this.mountedFolders.size > 0) {
130+
for (const [mountDir, folder] of this.mountedFolders) {
131+
try {
132+
const fsDir = URI.parse(folder.uri).fsPath;
133+
await this.workerManager.mountFolder(fsDir, mountDir);
134+
} catch (error) {
135+
this.logError(`remounting folder ${mountDir}`, error);
136+
}
137+
}
138+
}
139+
126140
this.status = STATUS.Initialized;
127141
this.telemetry.count('initialized', 1, { unit: '1' });
128142
} catch (error) {
@@ -147,8 +161,14 @@ export class CfnLintService implements SettingsConfigurable, Closeable {
147161
const fsDir = URI.parse(folder.uri).fsPath;
148162
const mountDir = '/'.concat(folder.name);
149163

164+
// Check if already mounted
165+
if (this.mountedFolders.has(mountDir)) {
166+
return;
167+
}
168+
150169
try {
151170
await this.workerManager.mountFolder(fsDir, mountDir);
171+
this.mountedFolders.set(mountDir, folder);
152172
this.telemetry.count('mount.success', 1, { unit: '1' });
153173
} catch (error) {
154174
this.logError('mounting folder', error);
@@ -260,6 +280,7 @@ export class CfnLintService implements SettingsConfigurable, Closeable {
260280
}
261281
}
262282
} catch (error) {
283+
this.status = STATUS.Uninitialized;
263284
const errorMessage = extractErrorMessage(error);
264285
this.logError(`linting ${fileType} by string`, error);
265286
this.publishErrorDiagnostics(uri, errorMessage);
@@ -299,6 +320,9 @@ export class CfnLintService implements SettingsConfigurable, Closeable {
299320
content: string,
300321
): Promise<void> {
301322
try {
323+
// Ensure folder is mounted before linting
324+
await this.mountFolder(folder);
325+
302326
const relativePath = uri.replace(folder.uri, '/'.concat(folder.name));
303327

304328
// Use worker to lint file
@@ -330,6 +354,7 @@ export class CfnLintService implements SettingsConfigurable, Closeable {
330354
}
331355
}
332356
} catch (error) {
357+
this.status = STATUS.Uninitialized;
333358
const errorMessage = extractErrorMessage(error);
334359
this.logError(`linting ${fileType} by file`, error);
335360
this.publishErrorDiagnostics(uri, errorMessage);

tst/unit/services/cfnLint/CfnLintService.test.ts

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -298,6 +298,35 @@ describe('CfnLintService', () => {
298298

299299
await expect(service.mountFolder(mockWorkspaceFolder)).rejects.toThrow('Failed to mount directory');
300300
});
301+
302+
test('should not mount folder twice', async () => {
303+
await service.initialize();
304+
305+
// Mount folder first time
306+
await service.mountFolder(mockWorkspaceFolder);
307+
expect(mockWorkerManager.mountFolder.callCount).toBe(1);
308+
309+
// Mount same folder again - should not call worker
310+
await service.mountFolder(mockWorkspaceFolder);
311+
expect(mockWorkerManager.mountFolder.callCount).toBe(1);
312+
});
313+
314+
test('should remount folders after worker recovery', async () => {
315+
await service.initialize();
316+
await service.mountFolder(mockWorkspaceFolder);
317+
expect(mockWorkerManager.mountFolder.callCount).toBe(1);
318+
319+
// Simulate worker crash - this sets status to uninitialized
320+
mockWorkerManager.lintTemplate.rejects(new Error('Worker crashed'));
321+
await service.lintDelayed(mockTemplate, mockUri, LintTrigger.OnSave);
322+
323+
// Reset mock to succeed and trigger recovery
324+
mockWorkerManager.lintTemplate.resolves([]);
325+
await service.lintDelayed(mockTemplate, mockUri, LintTrigger.OnSave);
326+
327+
// Should have remounted during recovery
328+
expect(mockWorkerManager.mountFolder.callCount).toBe(2);
329+
});
301330
});
302331

303332
describe('lint', () => {
@@ -352,6 +381,7 @@ describe('CfnLintService', () => {
352381
await service.lint(mockTemplate, mockUri);
353382

354383
expect(mockComponents.workspace.getWorkspaceFolder.calledWith(mockUri)).toBe(true);
384+
expect(mockWorkerManager.mountFolder.calledWith('/path/to/workspace/project', '/project')).toBe(true);
355385
expect(
356386
mockWorkerManager.lintFile.calledWith(
357387
'/project/template.yaml',
@@ -376,6 +406,30 @@ describe('CfnLintService', () => {
376406
).toBe(true);
377407
});
378408

409+
test('should handle worker crash during standalone linting', async () => {
410+
mockComponents.workspace.getWorkspaceFolder.returns(undefined);
411+
mockWorkerManager.lintTemplate.rejects(new Error('Worker exited unexpectedly with code 1'));
412+
413+
await service.lint(mockTemplate, mockUri);
414+
415+
// Should publish empty diagnostics and not crash
416+
expect(mockComponents.diagnosticCoordinator.publishDiagnostics.called).toBe(true);
417+
// Service should be marked as uninitialized for recovery
418+
expect(service.isInitialized()).toBe(false);
419+
});
420+
421+
test('should handle worker crash during workspace file linting', async () => {
422+
mockComponents.workspace.getWorkspaceFolder.returns(mockWorkspaceFolder);
423+
mockWorkerManager.lintFile.rejects(new Error('Worker exited unexpectedly with code 1'));
424+
425+
await service.lint(mockTemplate, mockUri);
426+
427+
// Should publish empty diagnostics and not crash
428+
expect(mockComponents.diagnosticCoordinator.publishDiagnostics.called).toBe(true);
429+
// Service should be marked as uninitialized for recovery
430+
expect(service.isInitialized()).toBe(false);
431+
});
432+
379433
test('should handle JSON templates', async () => {
380434
mockComponents.workspace.getWorkspaceFolder.returns(undefined);
381435

0 commit comments

Comments
 (0)