Skip to content

Commit 7c58b3a

Browse files
committed
fix(runner): deprecation race condition
1 parent 358d338 commit 7c58b3a

File tree

2 files changed

+82
-3
lines changed

2 files changed

+82
-3
lines changed

packages/cli-v3/src/entryPoints/managed/snapshot.test.ts

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -697,6 +697,65 @@ describe("SnapshotManager", () => {
697697
true
698698
);
699699
});
700+
701+
it("should handle deprecated snapshot race condition - avoid false positives from stale polls", async () => {
702+
const onSnapshotChange = vi.fn();
703+
704+
// Mock MetadataClient to simulate runner ID change (restore detected) on first call
705+
let isFirstCall = true;
706+
const mockMetadataClient = {
707+
getEnvOverrides: vi.fn().mockImplementation(() => {
708+
if (isFirstCall) {
709+
isFirstCall = false;
710+
return Promise.resolve([null, { TRIGGER_RUNNER_ID: "test-runner-2" }]); // Different runner ID = restore
711+
}
712+
return Promise.resolve([null, { TRIGGER_RUNNER_ID: "test-runner-2" }]); // Same runner ID afterward
713+
}),
714+
};
715+
716+
const manager = new SnapshotManager({
717+
runnerId: "test-runner-1",
718+
runFriendlyId: "test-run-1",
719+
initialSnapshotId: "snapshot-1",
720+
initialStatus: "EXECUTING_WITH_WAITPOINTS",
721+
logger: mockLogger,
722+
metadataClient: mockMetadataClient as any,
723+
onSnapshotChange,
724+
onSuspendable: mockSuspendableHandler,
725+
});
726+
727+
// First update: Process restore transition with deprecated statuses (normal case)
728+
// This simulates: EXECUTING_WITH_WAITPOINTS -> [SUSPENDED, QUEUED] -> PENDING_EXECUTING
729+
await manager.handleSnapshotChanges([
730+
createRunExecutionData({ snapshotId: "snapshot-suspended", executionStatus: "SUSPENDED" }),
731+
createRunExecutionData({ snapshotId: "snapshot-queued", executionStatus: "QUEUED" }),
732+
createRunExecutionData({ snapshotId: "snapshot-2", executionStatus: "PENDING_EXECUTING" }),
733+
]);
734+
735+
// First call should be deprecated=false (restore detected)
736+
expect(onSnapshotChange).toHaveBeenCalledWith(
737+
expect.objectContaining({ snapshot: expect.objectContaining({ friendlyId: "snapshot-2" }) }),
738+
false
739+
);
740+
741+
onSnapshotChange.mockClear();
742+
743+
// Second update: Should only get new snapshot (race condition case)
744+
// This simulates a stale poll that returns: getSnapshotsSince(snapshot-1) -> [SUSPENDED, QUEUED, snapshot-2, snapshot-3]
745+
// The SUSPENDED/QUEUED should be ignored as already seen
746+
await manager.handleSnapshotChanges([
747+
createRunExecutionData({ snapshotId: "snapshot-suspended", executionStatus: "SUSPENDED" }), // Already seen
748+
createRunExecutionData({ snapshotId: "snapshot-queued", executionStatus: "QUEUED" }), // Already seen
749+
createRunExecutionData({ snapshotId: "snapshot-2", executionStatus: "PENDING_EXECUTING" }), // Already processed
750+
createRunExecutionData({ snapshotId: "snapshot-3", executionStatus: "EXECUTING" }), // New
751+
]);
752+
753+
// Should call onSnapshotChange with deprecated = false (no new deprecated snapshots)
754+
expect(onSnapshotChange).toHaveBeenCalledWith(
755+
expect.objectContaining({ snapshot: expect.objectContaining({ friendlyId: "snapshot-3" }) }),
756+
false
757+
);
758+
});
700759
});
701760

702761
// Helper to generate RunExecutionData with sensible defaults

packages/cli-v3/src/entryPoints/managed/snapshot.ts

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,10 @@ export class SnapshotManager {
4949
private changeQueue: QueuedChangeItem[] = [];
5050
private isProcessingQueue = false;
5151

52+
// Track seen deprecated snapshots to prevent false positives
53+
private seenDeprecatedSnapshotIds: string[] = [];
54+
private readonly maxSeenDeprecatedSnapshotIds = 50;
55+
5256
constructor(opts: SnapshotManagerOptions) {
5357
this.runFriendlyId = opts.runFriendlyId;
5458
this.runnerId = opts.runnerId;
@@ -284,9 +288,13 @@ export class SnapshotManager {
284288

285289
// Check if any previous snapshot is QUEUED or SUSPENDED
286290
const deprecatedStatus: TaskRunExecutionStatus[] = ["QUEUED", "SUSPENDED"];
287-
const deprecatedSnapshots = previousSnapshots.filter((snap) =>
288-
deprecatedStatus.includes(snap.snapshot.executionStatus)
289-
);
291+
const deprecatedSnapshots = previousSnapshots.filter((snap) => {
292+
const isDeprecated = deprecatedStatus.includes(snap.snapshot.executionStatus);
293+
const previouslySeen = this.seenDeprecatedSnapshotIds.some(
294+
(s) => s === snap.snapshot.friendlyId
295+
);
296+
return isDeprecated && !previouslySeen;
297+
});
290298

291299
let deprecated = false;
292300
if (deprecatedSnapshots.length > 0) {
@@ -298,6 +306,18 @@ export class SnapshotManager {
298306
} else {
299307
deprecated = true;
300308
}
309+
310+
// Add the deprecated snapshot IDs to the seen list
311+
this.seenDeprecatedSnapshotIds.push(
312+
...deprecatedSnapshots.map((s) => s.snapshot.friendlyId)
313+
);
314+
315+
if (this.seenDeprecatedSnapshotIds.length > this.maxSeenDeprecatedSnapshotIds) {
316+
// Only keep the latest maxSeenDeprecatedSnapshotIds
317+
this.seenDeprecatedSnapshotIds = this.seenDeprecatedSnapshotIds.slice(
318+
-this.maxSeenDeprecatedSnapshotIds
319+
);
320+
}
301321
}
302322

303323
const { snapshot } = latestSnapshot;

0 commit comments

Comments
 (0)