Skip to content

fix(extensions): back up the extension dir before update so rollback restores it #29166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mahirhir wants to merge 1 commit into from
Closed

fix(extensions): back up the extension dir before update so rollback restores it #29166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion packages/cli/src/config/extensions/update.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,16 @@ describe('Extension Update Logic', () => {
),
).rejects.toThrow('Updated extension not found after installation');

expect(copyExtension).toHaveBeenCalledWith(
// Order matters. A backup taken after the install has already mutated
// the extension directory is exactly as empty as no backup at all, so
// an unordered pair of assertions would not catch a regression.
expect(copyExtension).toHaveBeenNthCalledWith(
1,
mockExtension.path,
'/tmp/mock-dir',
);
expect(copyExtension).toHaveBeenNthCalledWith(
2,
'/tmp/mock-dir',
mockExtension.path,
);
Expand All @@ -309,6 +318,27 @@ describe('Extension Update Logic', () => {
expect(fs.promises.rm).toHaveBeenCalled();
});

it('should not restore from the temp dir if the backup itself failed', async () => {
vi.mocked(copyExtension).mockRejectedValueOnce(
new Error('Backup failed'),
);

await expect(
updateExtension(
mockExtension,
mockExtensionManager,
ExtensionUpdateState.UPDATE_AVAILABLE,
mockDispatch,
),
).rejects.toThrow('Backup failed');

// The extension directory is still intact at this point. Copying a
// partial temp dir over it would be the corruption the rollback exists
// to prevent, so the only call must be the failed backup attempt.
expect(copyExtension).toHaveBeenCalledTimes(1);
expect(fs.promises.rm).toHaveBeenCalled();
});

describe('Integrity Verification', () => {
it('should fail update with security alert if integrity is invalid', async () => {
vi.mocked(
Expand Down
13 changes: 12 additions & 1 deletion packages/cli/src/config/extensions/update.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,16 @@ export async function updateExtension(
const originalVersion = extension.version;

const tempDir = await ExtensionStorage.createTmpDir();
// Tracks whether tempDir actually holds a copy of the current installation.
// The rollback below must not restore from tempDir unless it does, or a
// failed backup would overwrite an intact extension with a partial copy.
let backedUp = false;
try {
// Back up the current installation before anything mutates it. Without
// this, tempDir stays empty and the rollback in the catch block restores
// nothing.
await copyExtension(extension.path, tempDir);
backedUp = true;
const previousExtensionConfig = await extensionManager.loadExtensionConfig(
extension.path,
);
Expand Down Expand Up @@ -142,7 +151,9 @@ export async function updateExtension(
type: 'SET_STATE',
payload: { name: extension.name, state: ExtensionUpdateState.ERROR },
});
await copyExtension(tempDir, extension.path);
if (backedUp) {
await copyExtension(tempDir, extension.path);
}
Comment on lines +154 to +156

@gemini-code-assist gemini-code-assist Bot Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

When rolling back a failed update, copying the backup directory (tempDir) directly over the existing extension.path using copyExtension (which uses fs.promises.cp) will not delete any new files that were successfully written by the failed update before it crashed. This can leave the extension directory in a corrupted, 'dirty' state containing a mix of old and new files.

To ensure a clean and reliable rollback, the corrupted extension.path directory should be completely removed before restoring the backup. Additionally, wrapping the rollback in a try-catch block ensures that if the rollback itself fails, the original installation error e is still thrown and not shadowed by the rollback error.

    if (backedUp) {
      try {
        await fs.promises.rm(extension.path, { recursive: true, force: true });
        await copyExtension(tempDir, extension.path);
      } catch (rollbackError) {
        debugLogger.error(
          'Failed to rollback extension update: ' + getErrorMessage(rollbackError)
        );
      }
    }

throw e;
} finally {
await fs.promises.rm(tempDir, { recursive: true, force: true });
Expand Down