Skip to content

Commit 96f38ef

Browse files
StellaHuang95Copilot
StellaHuang95
and
Copilot
committed
Harden inline routing invalidation
Cancel stale reads after edits, preserve monotonic metadata revisions, and align BOM source offsets. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6b12d843-8011-4bfc-9ba9-f75761eadee2
1 parent e07a02b commit 96f38ef

7 files changed

Lines changed: 153 additions & 19 deletions

File tree

‎src/common/inlineScript/metadata.ts‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,9 @@ export interface InlineScriptMetadata {
2828
*/
2929
readonly range: { readonly start: number; readonly end: number };
3030
/**
31-
* Character offsets of the same metadata block in source text after the
32-
* parser's BOM handling. Unlike {@link range}, these preserve CRLF, so
33-
* they can be compared with TextDocument change offsets.
31+
* Character offsets of the same metadata block in the original source
32+
* text. Unlike {@link range}, these include a leading BOM and preserve
33+
* CRLF, so they can be compared with TextDocument change offsets.
3434
*
3535
* Optional to keep manually constructed metadata compatible; parser
3636
* results always supply it.
@@ -88,7 +88,8 @@ export function readInlineScriptMetadata(scriptText: string): InlineScriptMetada
8888
// "UTF-8 with BOM" on Windows have this; without stripping it the
8989
// first line becomes "\uFEFF# /// script" and the regex fails to
9090
// match.
91-
const sourceText = scriptText.charCodeAt(0) === 0xfeff ? scriptText.slice(1) : scriptText;
91+
const bomOffset = scriptText.charCodeAt(0) === 0xfeff ? 1 : 0;
92+
const sourceText = scriptText.slice(bomOffset);
9293
let text = sourceText;
9394

9495
// Normalize CRLF and lone CR to LF so the canonical regex (which
@@ -226,8 +227,8 @@ export function readInlineScriptMetadata(scriptText: string): InlineScriptMetada
226227
tool,
227228
range: { start: matchStart, end },
228229
sourceRange: {
229-
start: sourceOffsetForNormalizedOffset(sourceText, matchStart),
230-
end: sourceOffsetForNormalizedOffset(sourceText, end),
230+
start: bomOffset + sourceOffsetForNormalizedOffset(sourceText, matchStart),
231+
end: bomOffset + sourceOffsetForNormalizedOffset(sourceText, end),
231232
},
232233
};
233234
}

‎src/common/inlineScript/routingRegistry.ts‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ interface ScriptRoutingState {
3030

3131
export class InlineScriptRoutingRegistry implements Disposable {
3232
private readonly states = new Map<string, ScriptRoutingState>();
33+
private readonly metadataRevisions = new Map<string, number>();
3334
private readonly _onDidChangeRouteability = new EventEmitter<InlineScriptRouteabilityChangeEvent>();
3435
private readonly _onDidChangeMetadata = new EventEmitter<InlineScriptMetadataChangeEvent>();
3536

@@ -44,16 +45,16 @@ export class InlineScriptRoutingRegistry implements Disposable {
4445
return;
4546
}
4647
const metadataIdentity = getInlineScriptMetadataRoutingIdentity(metadata);
48+
const metadataRevision = this.nextMetadataRevision(scriptPath);
4749
this.update(
4850
scriptPath,
4951
(state) => {
50-
const currentRevision = state?.metadataRevision ?? 0;
5152
return {
5253
...state,
5354
uri,
5455
metadata,
5556
metadataIdentity,
56-
metadataRevision: currentRevision + 1,
57+
metadataRevision,
5758
};
5859
},
5960
true,
@@ -65,16 +66,16 @@ export class InlineScriptRoutingRegistry implements Disposable {
6566
if (!scriptPath) {
6667
return;
6768
}
69+
const metadataRevision = this.nextMetadataRevision(scriptPath);
6870
this.update(
6971
scriptPath,
7072
(state) => {
71-
const currentRevision = state?.metadataRevision ?? 0;
7273
return {
7374
...state,
7475
uri,
7576
metadata: undefined,
7677
metadataIdentity: undefined,
77-
metadataRevision: currentRevision + 1,
78+
metadataRevision,
7879
};
7980
},
8081
true,
@@ -93,7 +94,7 @@ export class InlineScriptRoutingRegistry implements Disposable {
9394

9495
public getMetadataRevision(script: Uri | string): number {
9596
const scriptPath = getInlineScriptRoutingKey(script);
96-
return scriptPath ? (this.states.get(scriptPath)?.metadataRevision ?? 0) : 0;
97+
return scriptPath ? (this.metadataRevisions.get(scriptPath) ?? 0) : 0;
9798
}
9899

99100
public getUri(script: Uri | string): Uri | undefined {
@@ -125,6 +126,7 @@ export class InlineScriptRoutingRegistry implements Disposable {
125126

126127
public dispose(): void {
127128
this.states.clear();
129+
this.metadataRevisions.clear();
128130
this._onDidChangeMetadata.dispose();
129131
this._onDidChangeRouteability.dispose();
130132
}
@@ -134,7 +136,10 @@ export class InlineScriptRoutingRegistry implements Disposable {
134136
updater: (state: ScriptRoutingState) => ScriptRoutingState,
135137
fireMetadataChange: boolean = false,
136138
): void {
137-
const previous = this.states.get(scriptPath) ?? { metadataRevision: 0, validatedAssociation: false };
139+
const previous = this.states.get(scriptPath) ?? {
140+
metadataRevision: this.metadataRevisions.get(scriptPath) ?? 0,
141+
validatedAssociation: false,
142+
};
138143
const previousRouteable = this.isRouteable(previous);
139144
const next = updater(previous);
140145

@@ -166,6 +171,12 @@ export class InlineScriptRoutingRegistry implements Disposable {
166171
private isRouteable(state: ScriptRoutingState | undefined): boolean {
167172
return !!state?.metadata && state.validatedAssociation;
168173
}
174+
175+
private nextMetadataRevision(scriptPath: string): number {
176+
const revision = (this.metadataRevisions.get(scriptPath) ?? 0) + 1;
177+
this.metadataRevisions.set(scriptPath, revision);
178+
return revision;
179+
}
169180
}
170181

171182
export function getInlineScriptRoutingKey(script: Uri | string): string | undefined {

‎src/features/inlineScript/lazyDetector.ts‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -244,13 +244,15 @@ export class InlineScriptLazyDetector implements Disposable {
244244
return;
245245
}
246246
if (this.routingRegistry) {
247+
const key = e.document.uri.toString();
247248
const metadata = this.routingRegistry.getMetadata(e.document.uri);
248249
if (
249-
metadata &&
250-
this.contentChangesMayAffectMetadata(
251-
e.contentChanges,
252-
metadata.sourceRange?.end ?? metadata.range.end,
253-
)
250+
(metadata &&
251+
this.contentChangesMayAffectMetadata(
252+
e.contentChanges,
253+
metadata.sourceRange?.end ?? metadata.range.end,
254+
)) ||
255+
(!metadata && this.inFlight.has(key))
254256
) {
255257
this.clearRouteability(e.document.uri);
256258
}
@@ -282,6 +284,10 @@ export class InlineScriptLazyDetector implements Disposable {
282284
if (!this.routingRegistry || !shouldTrackRoutingUri(uri)) {
283285
return;
284286
}
287+
const key = uri.toString();
288+
if (this.inFlight.has(key)) {
289+
this.advanceRoutingReadGeneration(key);
290+
}
285291
this.routingRegistry.clearMetadata(uri);
286292
this.routingRegistry.setValidatedAssociation(uri, false);
287293
}
@@ -314,7 +320,7 @@ export class InlineScriptLazyDetector implements Disposable {
314320
return;
315321
}
316322
this.inFlight.delete(key);
317-
if (routingGeneration !== undefined && this.routingReadGenerations.get(key) === routingGeneration) {
323+
if (routingGeneration !== undefined) {
318324
this.routingReadGenerations.delete(key);
319325
}
320326
}

‎src/test/common/inlineScript/metadata.unit.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ suite('inlineScriptMetadata', () => {
158158
assert.ok(md);
159159
assert.deepStrictEqual([...(md.dependencies ?? [])], ['a']);
160160
assert.strictEqual(md.range.start, 0, 'normalized parser offsets continue to exclude the BOM');
161-
assert.deepStrictEqual(md.sourceRange, { start: 0, end: text.length - 1 });
161+
assert.deepStrictEqual(md.sourceRange, { start: 1, end: text.length });
162162
});
163163

164164
test('shebang before block does not block detection', () => {

‎src/test/common/inlineScript/routingRegistry.unit.test.ts‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved.
2+
// Licensed under the MIT License.
3+
4+
import assert from 'assert';
5+
import { Uri } from 'vscode';
6+
import {
7+
InlineScriptRoutingRegistry,
8+
getInlineScriptMetadataRoutingIdentity,
9+
} from '../../../common/inlineScript/routingRegistry';
10+
11+
const METADATA = {
12+
requiresPython: '>=3.11',
13+
dependencies: ['requests'],
14+
range: { start: 0, end: 40 },
15+
};
16+
17+
suite('InlineScriptRoutingRegistry', () => {
18+
test('keeps metadata revisions monotonic after an empty state is removed', () => {
19+
const registry = new InlineScriptRoutingRegistry();
20+
const uri = Uri.file('/workspace/script.py');
21+
22+
registry.setMetadata(uri, METADATA);
23+
const firstRevision = registry.getMetadataRevision(uri);
24+
registry.clearMetadata(uri);
25+
const clearedRevision = registry.getMetadataRevision(uri);
26+
registry.setMetadata(uri, METADATA);
27+
const restoredRevision = registry.getMetadataRevision(uri);
28+
29+
assert.strictEqual(firstRevision, 1);
30+
assert.strictEqual(clearedRevision, 2);
31+
assert.strictEqual(restoredRevision, 3);
32+
assert.strictEqual(registry.getMetadataIdentity(uri), getInlineScriptMetadataRoutingIdentity(METADATA));
33+
registry.dispose();
34+
});
35+
});

‎src/test/features/inlineScript/lazyDetector.unit.test.ts‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,27 @@ suite('InlineScriptLazyDetector', () => {
346346
detector.dispose();
347347
});
348348

349+
test('a metadata edit invalidates an in-flight open read before metadata is registered', async () => {
350+
const uri = Uri.file(path.resolve('/ws/edit-race.py'));
351+
const staleRead = createDeferred<ism.InlineScriptMetadata>();
352+
readMetadataStub.returns(staleRead.promise);
353+
routingRegistry.setValidatedAssociation(uri, true);
354+
const detector = createDetector();
355+
356+
const open = openListener!(makeDoc(uri)) as Promise<void>;
357+
fireChange(uri, makeContentChanges(0));
358+
staleRead.resolve(VALID_METADATA);
359+
await open;
360+
361+
assert.strictEqual(routingRegistry.getMetadata(uri), undefined);
362+
assert.strictEqual(routingRegistry.shouldRoute(uri), false);
363+
assert.strictEqual(
364+
(detector as unknown as { routingReadGenerations: Map<string, number> }).routingReadGenerations.size,
365+
0,
366+
);
367+
detector.dispose();
368+
});
369+
349370
test('telemetry-only concurrent open + save still coalesces to a single read', async () => {
350371
const uri = Uri.file(path.resolve('/ws/telemetry-race.py'));
351372
readMetadataStub.resolves(VALID_METADATA);
@@ -413,6 +434,10 @@ suite('InlineScriptLazyDetector', () => {
413434

414435
fireChange(uri, makeContentChanges(0));
415436
assert.strictEqual(routingRegistry.shouldRoute(uri), false);
437+
assert.strictEqual(
438+
(detector as unknown as { routingReadGenerations: Map<string, number> }).routingReadGenerations.size,
439+
0,
440+
);
416441

417442
await fireSave(uri);
418443
assert.deepStrictEqual(routingRegistry.getMetadata(uri), VALID_METADATA);
@@ -465,6 +490,27 @@ suite('InlineScriptLazyDetector', () => {
465490
detector.dispose();
466491
});
467492

493+
test('invalidates routing for an edit at the end of a BOM-prefixed metadata block', async () => {
494+
const uri = Uri.file(path.resolve('/elsewhere/bom.py'));
495+
const source =
496+
'\uFEFF# /// script\r\n# dependencies = ["requests"]\r\n# ///\r\nprint("hello")\r\n';
497+
const metadata = ism.readInlineScriptMetadata(source);
498+
assert.ok(metadata?.sourceRange);
499+
assert.deepStrictEqual(metadata.sourceRange, {
500+
start: 1,
501+
end: source.indexOf('print'),
502+
});
503+
readMetadataStub.resolves(metadata);
504+
routingRegistry.setValidatedAssociation(uri, true);
505+
const detector = createDetector();
506+
507+
await fireOpen(uri);
508+
fireChange(uri, makeContentChanges(metadata.sourceRange.end - 1));
509+
510+
assert.strictEqual(routingRegistry.shouldRoute(uri), false);
511+
detector.dispose();
512+
});
513+
468514
test('save rehydrates routing from saved file metadata rather than the live buffer', async () => {
469515
const uri = Uri.file(path.resolve('/elsewhere/savedState.py'));
470516
readMetadataStub.resolves(VALID_METADATA);

‎src/test/managers/builtin/inlineScript/envManager.unit.test.ts‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4359,6 +4359,41 @@ suite('InlineScriptEnvManager', () => {
43594359
assert.strictEqual(refreshManager.lastValidatedMetadataIdentityProofs.has(scriptPath), false);
43604360
});
43614361

4362+
test('ignores a stale refresh when the same metadata returns after routeability is cleared', async () => {
4363+
const uri = scriptUri();
4364+
const scriptPath = normalizePath(uri.fsPath);
4365+
const environment = await createOwnedEnvironment();
4366+
await manager.set(uri, environment);
4367+
const refreshManager = asMetadataRefreshManager(manager);
4368+
refreshManager.subscriptions[0].dispose();
4369+
routingRegistry.setMetadata(uri, VALID_METADATA);
4370+
const metadataIdentity = routingRegistry.getMetadataIdentity(uri)!;
4371+
const staleRevision = routingRegistry.getMetadataRevision(uri);
4372+
let resolveProof: ((value: boolean) => void) | undefined;
4373+
const proofStub = sinon.stub(refreshManager, 'currentCacheEntryProvesSourceMetadataIdentity').returns(
4374+
new Promise<boolean>((resolve) => {
4375+
resolveProof = resolve;
4376+
}),
4377+
);
4378+
4379+
const pendingRefresh = refreshManager.refreshValidatedAssociationForMetadataInternal(
4380+
scriptPath,
4381+
uri,
4382+
VALID_METADATA,
4383+
metadataIdentity,
4384+
staleRevision,
4385+
refreshManager.associationRevisions.get(scriptPath) ?? 0,
4386+
);
4387+
await waitForStubCall(proofStub);
4388+
routingRegistry.clearMetadata(uri);
4389+
routingRegistry.setMetadata(uri, VALID_METADATA);
4390+
assert.ok(routingRegistry.getMetadataRevision(uri) > staleRevision);
4391+
resolveProof!(true);
4392+
await pendingRefresh;
4393+
4394+
assert.strictEqual(routingRegistry.hasValidatedAssociation(uri), false);
4395+
});
4396+
43624397
test('ignores a stale saved-metadata refresh when an unset wins while sidecar proof awaits', async () => {
43634398
const uri = scriptUri();
43644399
const scriptPath = normalizePath(uri.fsPath);

0 commit comments

Comments
 (0)