Skip to content

Commit 842816d

Browse files
committed
fix: filter cross-exercise build results via participation lookup
Artemis delivers build results for all of the user's participations over a single WebSocket topic, and ResultDTO carries only a participationId, not an exerciseId. TelemetryManager.onNewResult previously guarded only against a missing active session, so a result from exercise A arriving while exercise B was active would feed A's compiler errors into B's EQ engine — producing phantom struggle signals on the wrong exercise. - ExerciseRegistry now stores an optional participationId per exercise and exposes getExerciseIdByParticipation(). The reverse map is kept in sync on re-register (stale mappings dropped) and on clearCourse. - registerFromCourseData extracts studentParticipations[0].id, and ExerciseOpeningService forwards it on individual registrations. - TelemetryManager accepts an optional ExerciseRegistry and, on onNewResult, resolves result.participation.id → exerciseId. Known mismatches are dropped; unknown mappings are passed through (the registry may still be populating, and dropping real data is worse than a transient over-inclusive fallback). - extension.ts constructs ExerciseRegistry before TelemetryManager so the dependency can be injected at creation time. Drive-by fix in TelemetryManager.dispose(): the final _log call wrote to an already-disposed output channel, throwing "Channel has been closed". Moved the log before disposal — pre-existing bug that only surfaced once a test disposed a TelemetryManager in a teardown hook. Tests: 6 scenarios in telemetryManagerCrossExercise.test.ts cover match, mismatch, unknown, missing, no-session, and session-switch cases. 6 extra ExerciseRegistry tests cover the reverse-lookup contract including stale-mapping cleanup and course clearing.
1 parent fa4a679 commit 842816d

6 files changed

Lines changed: 253 additions & 13 deletions

File tree

extension/src/extension.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,10 @@ export async function activate(context: vscode.ExtensionContext) {
4545
const artemisApiService = new ArtemisApiService(authManager);
4646
const artemisWebsocketService = new ArtemisWebsocketService(authManager);
4747
const buildErrorCodeLensProvider = new BuildErrorCodeLensProvider();
48-
const telemetryManager = new TelemetryManager();
48+
// Created early because TelemetryManager needs it for participationId → exerciseId
49+
// resolution when filtering WebSocket build results (prevents cross-exercise contamination).
50+
const exerciseRegistry = new ExerciseRegistry();
51+
const telemetryManager = new TelemetryManager(exerciseRegistry);
4952
activeTelemetryManager = telemetryManager;
5053

5154
telemetryManager.setWebsocketService(artemisWebsocketService);
@@ -106,7 +109,7 @@ export async function activate(context: vscode.ExtensionContext) {
106109
});
107110

108111
// ── Registries & providers ───────────────────────────────────────
109-
const exerciseRegistry = new ExerciseRegistry();
112+
// Note: exerciseRegistry is created earlier (above) so TelemetryManager can use it.
110113
const courseDataCache = new CourseDataCache(artemisApiService);
111114
context.subscriptions.push(courseDataCache);
112115
const providerRegistry = createProviderRegistry();

extension/src/extension/services/exerciseRegistry.ts

Lines changed: 36 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,40 @@ import { logger } from './loggingService';
33

44
export interface ExerciseRegistryEntry extends ExerciseRef {
55
repositoryUri: string;
6+
participationId?: number;
67
}
78

89
export class ExerciseRegistry {
910
private exercises: Map<number, ExerciseRegistryEntry> = new Map();
11+
/**
12+
* Reverse lookup from participationId to exerciseId.
13+
* Enables TelemetryManager to filter WebSocket build results by the
14+
* currently-active exercise — the ResultDTO only carries a participationId,
15+
* not an exerciseId, so without this map a result from exercise A would
16+
* contaminate the EQ engine of the active exercise B.
17+
*/
18+
private participationToExercise: Map<number, number> = new Map();
1019

11-
public registerExercise(id: number, title: string, repositoryUri: string, shortName?: string, courseId?: number): void {
12-
this.exercises.set(id, { id, title, repositoryUri, shortName, courseId });
20+
public registerExercise(id: number, title: string, repositoryUri: string, shortName?: string, courseId?: number, participationId?: number): void {
21+
// If this exercise already had a different participationId, drop the old
22+
// reverse mapping so it doesn't linger and match stale results.
23+
const existing = this.exercises.get(id);
24+
if (existing?.participationId !== undefined && existing.participationId !== participationId) {
25+
this.participationToExercise.delete(existing.participationId);
26+
}
27+
this.exercises.set(id, { id, title, repositoryUri, shortName, courseId, participationId });
28+
if (participationId !== undefined) {
29+
this.participationToExercise.set(participationId, id);
30+
}
31+
}
32+
33+
/**
34+
* Resolve a participationId to the exerciseId it belongs to.
35+
* Returns undefined if the mapping is unknown (e.g. exercise was never
36+
* registered, or course data did not contain a participation).
37+
*/
38+
public getExerciseIdByParticipation(participationId: number): number | undefined {
39+
return this.participationToExercise.get(participationId);
1340
}
1441

1542
/**
@@ -25,6 +52,10 @@ export class ExerciseRegistry {
2552
}
2653
}
2754
for (const id of toDelete) {
55+
const entry = this.exercises.get(id);
56+
if (entry?.participationId !== undefined) {
57+
this.participationToExercise.delete(entry.participationId);
58+
}
2859
this.exercises.delete(id);
2960
}
3061
if (toDelete.length > 0) {
@@ -52,7 +83,7 @@ export class ExerciseRegistry {
5283
id?: number;
5384
title?: string;
5485
shortName?: string;
55-
studentParticipations?: Array<{ repositoryUri?: string }>
86+
studentParticipations?: Array<{ id?: number; repositoryUri?: string }>
5687
};
5788
const participations = ex.studentParticipations || [];
5889

@@ -62,7 +93,8 @@ export class ExerciseRegistry {
6293
ex.title,
6394
participations[0].repositoryUri,
6495
ex.shortName,
65-
courseId
96+
courseId,
97+
typeof participations[0].id === 'number' ? participations[0].id : undefined,
6698
);
6799
registeredCount++;
68100
registered.push(`${ex.id}: ${ex.title}`);

extension/src/extension/services/telemetry/telemetryManager.ts

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import { ArtemisWebsocketService } from '../websocket/artemisWebsocketService';
2323
import { ResultDTO, WebSocketMessageHandler } from '../../types';
2424
import { VSCODE_CONFIG } from '../../utils/constants';
2525
import { logger, LogCategory } from '../loggingService';
26+
import type { ExerciseRegistry } from '../exerciseRegistry';
2627

2728
/**
2829
* Central orchestration service for EQ-based struggle detection.
@@ -56,6 +57,7 @@ export class TelemetryManager implements vscode.Disposable, WebSocketMessageHand
5657
private readonly _sessionServices: SessionResettable[];
5758
private _activeExerciseId: number | undefined;
5859
private _lastTriggerType: TriggerType | undefined;
60+
private readonly _exerciseRegistry: ExerciseRegistry | undefined;
5961
// Debug mode
6062
private _debugMode: boolean = false;
6163
private _debugStatusBarItem: vscode.StatusBarItem;
@@ -84,7 +86,8 @@ export class TelemetryManager implements vscode.Disposable, WebSocketMessageHand
8486
return this._interventionService.onDidDismissIntervention;
8587
}
8688

87-
constructor() {
89+
constructor(exerciseRegistry?: ExerciseRegistry) {
90+
this._exerciseRegistry = exerciseRegistry;
8891
this._outputChannel = vscode.window.createOutputChannel('Artemis Telemetry');
8992
this._disposables.push(this._outputChannel);
9093

@@ -161,12 +164,15 @@ export class TelemetryManager implements vscode.Disposable, WebSocketMessageHand
161164

162165
this.endCurrentSession();
163166

167+
// Log BEFORE disposing the output channel — otherwise _log() writes to
168+
// an already-disposed channel and throws "Channel has been closed".
169+
this._log('TelemetryManager disposed');
170+
164171
while (this._disposables.length > 0) {
165172
this._disposables.pop()?.dispose();
166173
}
167174

168175
this._onDidCalculateEQ.dispose();
169-
this._log('TelemetryManager disposed');
170176
}
171177

172178
// ==================== WebSocket Message Handler ====================
@@ -183,14 +189,27 @@ export class TelemetryManager implements vscode.Disposable, WebSocketMessageHand
183189
return;
184190
}
185191

186-
// Guard: Skip results when no exercise session is active (Edge Case 1b).
187-
// ResultDTO has no exerciseId field, so we can't filter by exercise directly.
188-
// The WebSocket subscription (personalResults) may deliver results for any exercise.
189-
// Without an active session, results should not feed into EQ.
192+
// Guard 1: Skip results when no exercise session is active (Edge Case 1b).
193+
// The WebSocket subscription (personalResults) delivers results for any
194+
// participation of the user, not just the active exercise's.
190195
if (this._activeExerciseId === undefined) {
191196
return;
192197
}
193198

199+
// Guard 2: Skip results that belong to a different exercise than the
200+
// active session. ResultDTO only carries a participationId, so we
201+
// resolve it through ExerciseRegistry. Policy: permissive on unknown
202+
// mapping — if the registry has not yet learned this participationId
203+
// (e.g. first course load not finished), we let the result through
204+
// rather than dropping real data. Known mismatches are dropped.
205+
const resultParticipationId = result.participation?.id;
206+
if (resultParticipationId !== undefined && this._exerciseRegistry) {
207+
const mappedExerciseId = this._exerciseRegistry.getExerciseIdByParticipation(resultParticipationId);
208+
if (mappedExerciseId !== undefined && mappedExerciseId !== this._activeExerciseId) {
209+
return;
210+
}
211+
}
212+
194213
// Step 1: EQ snapshot FIRST (synchronous)
195214
const event = this._compileEmitter.handleBuildResult(result);
196215
if (event) {

extension/src/extension/services/ui/exerciseOpeningService.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,8 @@ export class ExerciseOpeningService {
3131
exerciseTitle,
3232
participations[0].repositoryUri,
3333
exercise.shortName || '',
34-
exercise.course?.id
34+
exercise.course?.id,
35+
typeof participations[0].id === 'number' ? participations[0].id : undefined,
3536
);
3637
logger.exercise(`Registered individual exercise: ${exerciseTitle}`);
3738
}

extension/test/unit/provider/exerciseRegistry.test.ts

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,55 @@ suite('ExerciseRegistry Test Suite', () => {
5757
assert.strictEqual(exercises[0].courseId, 200);
5858
});
5959

60+
// --- participationId reverse-lookup (NEW-2 fix) ---
61+
62+
test('getExerciseIdByParticipation returns exerciseId for registered participation', () => {
63+
registry.registerExercise(1, 'Ex1', 'https://git.example.com/ex1', 'ex1', 100, 5001);
64+
assert.strictEqual(registry.getExerciseIdByParticipation(5001), 1);
65+
});
66+
67+
test('getExerciseIdByParticipation returns undefined for unknown participation', () => {
68+
registry.registerExercise(1, 'Ex1', 'https://git.example.com/ex1', 'ex1', 100, 5001);
69+
assert.strictEqual(registry.getExerciseIdByParticipation(9999), undefined);
70+
});
71+
72+
test('getExerciseIdByParticipation returns undefined when exercise registered without participationId', () => {
73+
registry.registerExercise(1, 'Ex1', 'https://git.example.com/ex1', 'ex1', 100);
74+
assert.strictEqual(registry.getExerciseIdByParticipation(5001), undefined);
75+
});
76+
77+
test('re-registering same exercise with new participationId removes old reverse mapping', () => {
78+
registry.registerExercise(1, 'Ex1', 'https://git.example.com/ex1', 'ex1', 100, 5001);
79+
registry.registerExercise(1, 'Ex1', 'https://git.example.com/ex1', 'ex1', 100, 5002);
80+
assert.strictEqual(registry.getExerciseIdByParticipation(5001), undefined, 'old participation must no longer resolve');
81+
assert.strictEqual(registry.getExerciseIdByParticipation(5002), 1, 'new participation must resolve');
82+
});
83+
84+
test('clearCourse removes participation reverse mappings for that course', () => {
85+
registry.registerExercise(1, 'Ex1', 'https://git.example.com/ex1', 'ex1', 100, 5001);
86+
registry.registerExercise(2, 'Ex2', 'https://git.example.com/ex2', 'ex2', 200, 5002);
87+
registry.clearCourse(100);
88+
assert.strictEqual(registry.getExerciseIdByParticipation(5001), undefined, 'cleared course participation must not resolve');
89+
assert.strictEqual(registry.getExerciseIdByParticipation(5002), 2, 'other course participation must still resolve');
90+
});
91+
92+
test('registerFromCourseData extracts participationId from studentParticipations', () => {
93+
const courseData = {
94+
course: {
95+
id: 100,
96+
exercises: [
97+
{
98+
id: 1,
99+
title: 'Ex1',
100+
studentParticipations: [{ id: 5001, repositoryUri: 'https://git.example.com/ex1' }]
101+
}
102+
]
103+
}
104+
};
105+
registry.registerFromCourseData(courseData);
106+
assert.strictEqual(registry.getExerciseIdByParticipation(5001), 1);
107+
});
108+
60109
test('should replace course exercises when re-registering from fresh course data', () => {
61110
// Initial registration
62111
const courseData1 = {
Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
/**
2+
* Regression tests for NEW-2: Cross-exercise build result contamination.
3+
*
4+
* Background: ResultDTO from the Artemis WebSocket carries only a participationId,
5+
* never an exerciseId. Without a reverse lookup, a build result from exercise A
6+
* would be fed into the EQ engine of the currently-active exercise B, producing
7+
* phantom struggle signals.
8+
*
9+
* Fix: TelemetryManager accepts an ExerciseRegistry, and onNewResult resolves
10+
* result.participation.id → exerciseId via the registry. Mismatches are dropped.
11+
* Unknown mappings are passed through (permissive fallback so real data is not
12+
* lost while the registry is still populating).
13+
*/
14+
15+
import * as assert from 'assert';
16+
import { TelemetryManager } from '../../../src/extension/services/telemetry/telemetryManager';
17+
import { ExerciseRegistry } from '../../../src/extension/services/exerciseRegistry';
18+
import type { ResultDTO } from '../../../src/extension/domain';
19+
20+
function makeResult(participationId: number | undefined, opts: { buildFailed?: boolean; successful?: boolean } = {}): ResultDTO {
21+
return {
22+
id: Math.floor(Math.random() * 1_000_000),
23+
participation: participationId !== undefined ? { id: participationId } : undefined,
24+
submission: opts.buildFailed !== undefined ? { buildFailed: opts.buildFailed } : undefined,
25+
successful: opts.successful,
26+
};
27+
}
28+
29+
suite('TelemetryManager Cross-Exercise Filter (NEW-2 fix)', () => {
30+
let registry: ExerciseRegistry;
31+
let telemetry: TelemetryManager;
32+
33+
setup(() => {
34+
registry = new ExerciseRegistry();
35+
// Exercise A: exerciseId=1, participationId=5001
36+
// Exercise B: exerciseId=2, participationId=5002
37+
registry.registerExercise(1, 'Exercise A', 'git://a', 'exA', 100, 5001);
38+
registry.registerExercise(2, 'Exercise B', 'git://b', 'exB', 100, 5002);
39+
telemetry = new TelemetryManager(registry);
40+
});
41+
42+
teardown(() => {
43+
telemetry.dispose();
44+
});
45+
46+
// Note: a failing build result causes *two* EQ fires — one with source='build'
47+
// (from the snapshot add path) and one with source='trigger' (from the
48+
// execution-error BoundaryTrigger). We count them together and only assert
49+
// on the delta, so the guard logic is tested independently of that coupling.
50+
51+
test('result for active exercise is processed', () => {
52+
telemetry.startExerciseSession(2); // active = B
53+
let fireCount = 0;
54+
const sub = telemetry.onDidCalculateEQ(() => { fireCount++; });
55+
try {
56+
telemetry.onNewResult(makeResult(5002, { buildFailed: true }));
57+
assert.ok(fireCount > 0, 'EQ should update for active-exercise result');
58+
} finally {
59+
sub.dispose();
60+
}
61+
});
62+
63+
test('result for a different registered exercise is dropped', () => {
64+
telemetry.startExerciseSession(2); // active = B
65+
let fireCount = 0;
66+
const sub = telemetry.onDidCalculateEQ(() => { fireCount++; });
67+
try {
68+
// Result belongs to A's participation while B is active → must be dropped
69+
telemetry.onNewResult(makeResult(5001, { buildFailed: true }));
70+
assert.strictEqual(fireCount, 0, 'EQ must not update for foreign-exercise result');
71+
} finally {
72+
sub.dispose();
73+
}
74+
});
75+
76+
test('result with unknown participation is passed through (permissive fallback)', () => {
77+
telemetry.startExerciseSession(2);
78+
let fireCount = 0;
79+
const sub = telemetry.onDidCalculateEQ(() => { fireCount++; });
80+
try {
81+
// participationId 9999 is not in the registry → permissive: let it through
82+
telemetry.onNewResult(makeResult(9999, { buildFailed: true }));
83+
assert.ok(fireCount > 0, 'unknown mapping must not drop data');
84+
} finally {
85+
sub.dispose();
86+
}
87+
});
88+
89+
test('result without participation is passed through (permissive fallback)', () => {
90+
telemetry.startExerciseSession(2);
91+
let fireCount = 0;
92+
const sub = telemetry.onDidCalculateEQ(() => { fireCount++; });
93+
try {
94+
telemetry.onNewResult(makeResult(undefined, { buildFailed: true }));
95+
assert.ok(fireCount > 0, 'missing participation must not drop data');
96+
} finally {
97+
sub.dispose();
98+
}
99+
});
100+
101+
test('result is dropped when no session is active regardless of participation', () => {
102+
// No startExerciseSession call
103+
let fireCount = 0;
104+
const sub = telemetry.onDidCalculateEQ(() => { fireCount++; });
105+
try {
106+
telemetry.onNewResult(makeResult(5002, { buildFailed: true }));
107+
assert.strictEqual(fireCount, 0, 'no active session → no EQ update');
108+
} finally {
109+
sub.dispose();
110+
}
111+
});
112+
113+
test('after session switch, only results for new active exercise are accepted', () => {
114+
telemetry.startExerciseSession(1); // active = A
115+
let fireCount = 0;
116+
const sub = telemetry.onDidCalculateEQ(() => { fireCount++; });
117+
try {
118+
// A-result → accepted
119+
telemetry.onNewResult(makeResult(5001, { buildFailed: true }));
120+
const afterA = fireCount;
121+
assert.ok(afterA > 0, 'A-result on active session A must fire EQ');
122+
123+
telemetry.startExerciseSession(2); // switch to B
124+
125+
// Stale A-result arriving after switch → must be dropped
126+
telemetry.onNewResult(makeResult(5001, { buildFailed: true }));
127+
assert.strictEqual(fireCount, afterA, 'stale A-result after switch must be dropped');
128+
129+
// B-result → accepted
130+
telemetry.onNewResult(makeResult(5002, { buildFailed: true }));
131+
assert.ok(fireCount > afterA, 'B-result on active session B must fire EQ');
132+
} finally {
133+
sub.dispose();
134+
}
135+
});
136+
});

0 commit comments

Comments
 (0)