Skip to content

Commit 50b9957

Browse files
EhabYEhabY
authored
Use per-deployment keys for race-free deployment tracking (#747)
Replace shared deployment usage list with per-deployment access timestamps and derive known hostnames from secrets.keys(). This eliminates race conditions when multiple VS Code windows access simultaneously.
1 parent c565edb commit 50b9957

File tree

3 files changed

+108
-81
lines changed

3 files changed

+108
-81
lines changed

‎src/core/secretsManager.ts‎

Lines changed: 39 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -8,18 +8,16 @@ import type { Memento, SecretStorage, Disposable } from "vscode";
88

99
import type { Logger } from "../logging/logger";
1010

11-
// Each deployment has its own key to ensure atomic operations (multiple windows
12-
// writing to a shared key could drop data) and to receive proper VS Code events.
11+
// Per-deployment keys ensure atomic operations (multiple windows writing to a
12+
// shared key could drop data) and enable proper VS Code change events.
1313
const SESSION_KEY_PREFIX = "coder.session.";
1414
const OAUTH_CLIENT_PREFIX = "coder.oauth.client.";
15+
const DEPLOYMENT_ACCESS_PREFIX = "coder.access.";
1516

1617
type SecretKeyPrefix = typeof SESSION_KEY_PREFIX | typeof OAUTH_CLIENT_PREFIX;
1718

1819
const OAUTH_CALLBACK_KEY = "coder.oauthCallback";
19-
2020
const CURRENT_DEPLOYMENT_KEY = "coder.currentDeployment";
21-
22-
const DEPLOYMENT_USAGE_KEY = "coder.deploymentUsage";
2321
const DEFAULT_MAX_DEPLOYMENTS = 10;
2422

2523
const LEGACY_SESSION_TOKEN_KEY = "sessionToken";
@@ -53,14 +51,6 @@ const SessionAuthSchema = z.object({
5351

5452
export type SessionAuth = z.infer<typeof SessionAuthSchema>;
5553

56-
// Tracks when a deployment was last accessed for LRU pruning.
57-
const DeploymentUsageSchema = z.object({
58-
safeHostname: z.string(),
59-
lastAccessedAt: z.string(),
60-
});
61-
62-
type DeploymentUsage = z.infer<typeof DeploymentUsageSchema>;
63-
6454
const OAuthCallbackDataSchema = z.object({
6555
state: z.string(),
6656
code: z.string().nullable(),
@@ -224,61 +214,63 @@ export class SecretsManager {
224214
}
225215

226216
/**
227-
* Record that a deployment was accessed, moving it to the front of the LRU list.
217+
* Record that a deployment was accessed by updating its timestamp.
228218
* Prunes deployments beyond maxCount, clearing their auth data.
229219
*/
230220
public async recordDeploymentAccess(
231221
safeHostname: string,
232222
maxCount = DEFAULT_MAX_DEPLOYMENTS,
233223
): Promise<void> {
234-
const usage = this.getDeploymentUsage();
235-
const filtered = usage.filter((u) => u.safeHostname !== safeHostname);
236-
const newEntry = DeploymentUsageSchema.parse({
237-
safeHostname,
238-
lastAccessedAt: new Date().toISOString(),
239-
});
240-
filtered.unshift(newEntry);
241-
242-
const toKeep = filtered.slice(0, maxCount);
243-
const toRemove = filtered.slice(maxCount);
244-
245-
await Promise.all(
246-
toRemove.map((u) => this.clearAllAuthData(u.safeHostname)),
224+
// Update this deployment's access timestamp
225+
await this.memento.update(
226+
`${DEPLOYMENT_ACCESS_PREFIX}${safeHostname}`,
227+
new Date().toISOString(),
247228
);
248-
await this.memento.update(DEPLOYMENT_USAGE_KEY, toKeep);
229+
230+
// Prune if needed - errors here shouldn't block deployment access
231+
try {
232+
const allHostnames = await this.getKnownSafeHostnames();
233+
const toRemove = allHostnames.slice(maxCount);
234+
await Promise.all(toRemove.map((h) => this.clearAllAuthData(h)));
235+
} catch {
236+
this.logger.warn("Failed to prune old deployments");
237+
}
249238
}
250239

251240
/**
252-
* Clear all auth data for a deployment and remove it from the usage list.
241+
* Clear all auth data for a deployment, including the access timestamp.
253242
*/
254243
public async clearAllAuthData(safeHostname: string): Promise<void> {
255244
await Promise.all([
256245
this.clearSessionAuth(safeHostname),
257246
this.clearOAuthClientRegistration(safeHostname),
247+
this.memento.update(
248+
`${DEPLOYMENT_ACCESS_PREFIX}${safeHostname}`,
249+
undefined,
250+
),
258251
]);
259-
const usage = this.getDeploymentUsage().filter(
260-
(u) => u.safeHostname !== safeHostname,
261-
);
262-
await this.memento.update(DEPLOYMENT_USAGE_KEY, usage);
263252
}
264253

265254
/**
266255
* Get all known hostnames, ordered by most recently accessed.
256+
* Derives the list from actual session secrets stored.
267257
*/
268-
public getKnownSafeHostnames(): string[] {
269-
return this.getDeploymentUsage().map((u) => u.safeHostname);
270-
}
271-
272-
/**
273-
* Get the full deployment usage list with access timestamps.
274-
*/
275-
private getDeploymentUsage(): DeploymentUsage[] {
276-
const data = this.memento.get<unknown>(DEPLOYMENT_USAGE_KEY);
277-
if (!data) {
278-
return [];
279-
}
280-
const result = z.array(DeploymentUsageSchema).safeParse(data);
281-
return result.success ? result.data : [];
258+
public async getKnownSafeHostnames(): Promise<string[]> {
259+
const keys = await this.secrets.keys();
260+
const sessionHostnames = keys
261+
.filter((k) => k.startsWith(SESSION_KEY_PREFIX))
262+
.map((k) => k.slice(SESSION_KEY_PREFIX.length));
263+
264+
// Sort by access timestamp (most recent first)
265+
const withTimestamps = sessionHostnames.map((hostname) => ({
266+
hostname,
267+
accessedAt:
268+
this.memento.get<string>(`${DEPLOYMENT_ACCESS_PREFIX}${hostname}`) ??
269+
"",
270+
}));
271+
272+
withTimestamps.sort((a, b) => b.accessedAt.localeCompare(a.accessedAt));
273+
return withTimestamps.map((w) => w.hostname);
282274
}
283275

284276
/**

‎src/extension.ts‎

Lines changed: 28 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import { AuthInterceptor } from "./api/authInterceptor";
1111
import { CoderApi } from "./api/coderApi";
1212
import { Commands } from "./commands";
1313
import { ServiceContainer } from "./core/container";
14-
import { type SecretsManager } from "./core/secretsManager";
1514
import { DeploymentManager } from "./deployment/deploymentManager";
1615
import { CertificateError } from "./error/certificateError";
1716
import { getErrorDetail, toError } from "./error/errorUtils";
@@ -243,7 +242,7 @@ export async function activate(ctx: vscode.ExtensionContext): Promise<void> {
243242
showTreeViewSearch(ALL_WORKSPACES_TREE_ID),
244243
),
245244
vscode.commands.registerCommand("coder.debug.listDeployments", () =>
246-
listStoredDeployments(secretsManager),
245+
listStoredDeployments(serviceContainer),
247246
),
248247
);
249248

@@ -387,26 +386,36 @@ async function showTreeViewSearch(id: string): Promise<void> {
387386
}
388387

389388
async function listStoredDeployments(
390-
secretsManager: SecretsManager,
389+
serviceContainer: ServiceContainer,
391390
): Promise<void> {
392-
const hostnames = secretsManager.getKnownSafeHostnames();
393-
if (hostnames.length === 0) {
394-
vscode.window.showInformationMessage("No deployments stored.");
395-
return;
396-
}
391+
const secretsManager = serviceContainer.getSecretsManager();
392+
const output = serviceContainer.getLogger();
397393

398-
const selected = await vscode.window.showQuickPick(
399-
hostnames.map((hostname) => ({
400-
label: hostname,
401-
description: "Click to forget",
402-
})),
403-
{ placeHolder: "Select a deployment to forget" },
404-
);
394+
try {
395+
const hostnames = await secretsManager.getKnownSafeHostnames();
396+
if (hostnames.length === 0) {
397+
vscode.window.showInformationMessage("No deployments stored.");
398+
return;
399+
}
400+
401+
const selected = await vscode.window.showQuickPick(
402+
hostnames.map((hostname) => ({
403+
label: hostname,
404+
description: "Click to forget",
405+
})),
406+
{ placeHolder: "Select a deployment to forget" },
407+
);
405408

406-
if (selected) {
407-
await secretsManager.clearAllAuthData(selected.label);
408-
vscode.window.showInformationMessage(
409-
`Cleared auth data for ${selected.label}`,
409+
if (selected) {
410+
await secretsManager.clearAllAuthData(selected.label);
411+
vscode.window.showInformationMessage(
412+
`Cleared auth data for ${selected.label}`,
413+
);
414+
}
415+
} catch (error: unknown) {
416+
output.error("Failed to list stored deployments", error);
417+
vscode.window.showErrorMessage(
418+
"Failed to list stored deployments. Storage may be corrupted.",
410419
);
411420
}
412421
}

‎test/unit/core/secretsManager.test.ts‎

Lines changed: 41 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -75,72 +75,103 @@ describe("SecretsManager", () => {
7575
});
7676

7777
it("should track known safe hostnames", async () => {
78-
expect(secretsManager.getKnownSafeHostnames()).toEqual([]);
78+
expect(await secretsManager.getKnownSafeHostnames()).toEqual([]);
7979

8080
await secretsManager.setSessionAuth("example.com", {
8181
url: "https://example.com",
8282
token: "test-token",
8383
});
84-
expect(secretsManager.getKnownSafeHostnames()).toContain("example.com");
84+
expect(await secretsManager.getKnownSafeHostnames()).toContain(
85+
"example.com",
86+
);
8587

8688
await secretsManager.setSessionAuth("other-com", {
8789
url: "https://other.com",
8890
token: "other-token",
8991
});
90-
expect(secretsManager.getKnownSafeHostnames()).toContain("example.com");
91-
expect(secretsManager.getKnownSafeHostnames()).toContain("other-com");
92+
expect(await secretsManager.getKnownSafeHostnames()).toContain(
93+
"example.com",
94+
);
95+
expect(await secretsManager.getKnownSafeHostnames()).toContain(
96+
"other-com",
97+
);
9298
});
9399

94100
it("should remove safe hostname on clearAllAuthData", async () => {
95101
await secretsManager.setSessionAuth("example.com", {
96102
url: "https://example.com",
97103
token: "test-token",
98104
});
99-
expect(secretsManager.getKnownSafeHostnames()).toContain("example.com");
105+
expect(await secretsManager.getKnownSafeHostnames()).toContain(
106+
"example.com",
107+
);
100108

101109
await secretsManager.clearAllAuthData("example.com");
102-
expect(secretsManager.getKnownSafeHostnames()).not.toContain(
110+
expect(await secretsManager.getKnownSafeHostnames()).not.toContain(
103111
"example.com",
104112
);
105113
});
106114

115+
it("should throw when secrets storage is corrupted", async () => {
116+
await secretsManager.setSessionAuth("example.com", {
117+
url: "https://example.com",
118+
token: "test-token",
119+
});
120+
secretStorage.corruptStorage();
121+
122+
await expect(secretsManager.getKnownSafeHostnames()).rejects.toThrow();
123+
});
124+
107125
it("should order safe hostnames by most recently accessed", async () => {
126+
vi.useFakeTimers();
127+
108128
await secretsManager.setSessionAuth("first.com", {
109129
url: "https://first.com",
110130
token: "token1",
111131
});
132+
vi.advanceTimersByTime(10);
112133
await secretsManager.setSessionAuth("second.com", {
113134
url: "https://second.com",
114135
token: "token2",
115136
});
137+
vi.advanceTimersByTime(10);
116138
await secretsManager.setSessionAuth("first.com", {
117139
url: "https://first.com",
118140
token: "token1-updated",
119141
});
120142

121-
expect(secretsManager.getKnownSafeHostnames()).toEqual([
143+
expect(await secretsManager.getKnownSafeHostnames()).toEqual([
122144
"first.com",
123145
"second.com",
124146
]);
147+
148+
vi.useRealTimers();
125149
});
126150

127151
it("should prune old deployments when exceeding maxCount", async () => {
152+
// Use fake timers to ensure distinct timestamps for each host
153+
vi.useFakeTimers();
154+
128155
for (let i = 1; i <= 5; i++) {
129156
await secretsManager.setSessionAuth(`host${i}.com`, {
130157
url: `https://host${i}.com`,
131158
token: `token${i}`,
132159
});
160+
vi.advanceTimersByTime(10);
133161
}
134162

135-
await secretsManager.recordDeploymentAccess("new.com", 3);
163+
// With maxCount=3, only the 3 most recently accessed hosts should remain.
164+
await secretsManager.recordDeploymentAccess("host5.com", 3);
136165

137-
expect(secretsManager.getKnownSafeHostnames()).toEqual([
138-
"new.com",
166+
expect(await secretsManager.getKnownSafeHostnames()).toEqual([
139167
"host5.com",
140168
"host4.com",
169+
"host3.com",
141170
]);
142171
expect(await secretsManager.getSessionAuth("host1.com")).toBeUndefined();
143172
expect(await secretsManager.getSessionAuth("host2.com")).toBeUndefined();
173+
174+
vi.useRealTimers();
144175
});
145176
});
146177

@@ -382,11 +413,6 @@ describe("SecretsManager", () => {
382413
);
383414
expect(await secretsManager.getCurrentDeployment()).toBeNull();
384415
});
385-
386-
it("returns empty array for invalid deployment usage data", async () => {
387-
await memento.update("coder.deploymentUsage", [{ safeHostname: 123 }]);
388-
expect(secretsManager.getKnownSafeHostnames()).toEqual([]);
389-
});
390416
});
391417

392418
describe("backwards compatibility", () => {

0 commit comments

Comments
 (0)