Skip to content

Conversation

@maxbrunsfeld
Copy link
Collaborator

@maxbrunsfeld maxbrunsfeld commented Aug 21, 2025

This is another pure refactor, to prepare for adding direct WSL support.

Todo

  • Represent paths in the same way for all workspaces, instead of having a completely separate SSH representation
  • Adjust sqlite tables
    • ssh_projects -> ssh_connections (drop paths)
    • workspaces.local_paths -> paths
    • remove duplicate path columns on workspaces
  • Add migrations for backward-compatibility

Release Notes:

  • N/A

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Aug 21, 2025
@maxbrunsfeld maxbrunsfeld merged commit f649c31 into main Aug 22, 2025
22 checks passed
@maxbrunsfeld maxbrunsfeld deleted the ssh-remote-client-refactor branch August 22, 2025 21:10
maxbrunsfeld added a commit that referenced this pull request Aug 22, 2025
orual pushed a commit to orual/zed that referenced this pull request Aug 23, 2025
… other non-ssh remote projects (zed-industries#36714)

This is another pure refactor, to prepare for adding direct WSL support.

###  Todo

* [x] Represent `paths` in the same way for all workspaces, instead of
having a completely separate SSH representation
* [x] Adjust sqlite tables
    * [x] `ssh_projects` -> `ssh_connections` (drop paths)
    * [x] `workspaces.local_paths` -> `paths`
    * [x] remove duplicate path columns on `workspaces`
* [x] Add migrations for backward-compatibility

Release Notes:

- N/A

---------

Co-authored-by: Mikayla Maki <[email protected]>
orual pushed a commit to orual/zed that referenced this pull request Aug 23, 2025
maxbrunsfeld added a commit that referenced this pull request Aug 26, 2025
This fixes a regression on nightly introduced in
#36714

Release Notes:

- N/A
maxbrunsfeld added a commit that referenced this pull request Sep 6, 2025
Closes #37473

### Background

Previously, we enabled foreign keys at all times for our sqlite database
that we use for client-side state.
The problem with this is that In sqlite, `alter table` is somewhat
limited, so for many migrations, you must *recreate* the table: create a
new table called e.g. `workspace__2`, then copy all of the data from
`workspaces` into `workspace__2`, then delete the old `workspaces` table
and rename `workspaces__2` to `workspaces`. The way foreign keys work in
sqlite, when we delete the old table, all of its associated records in
other tables will be deleted due to `on delete cascade` clauses.

Unfortunately, one of the types of associated records that can be
deleted are `editors`, which sometimes store unsaved text. It is very
bad to delete these records, as they are the *only* place that this
unsaved text is stored.

This has already happened multiple times as we have migrated tables as
we develop Zed, but I caused it to happened again in
#36714.

### The Fix

The Sqlite docs recommend a multi-step approach to migrations where you:

* disable foreign keys
* start a transaction
* create a new table
* populate the new table with data from the old table
* delete the old table
* rename the new table to the old name
* run a foreign key check
* if it passes, commit the transaction
* enable foreign keys

In this PR, I've adjusted our sqlite migration code path to follow this
pattern more closely. Specifically, we disable foreign key checks before
running migrations, run a foreign key check before committing, and then
enable foreign key checks after the migrations are done.

In addition, I've added a generic query that we run *before* running the
foreign key check that explicitly deletes any rows that have dangling
foreign keys. This way, we avoid failing the migration (and breaking the
app) if a migration deletes data that *does* cause associated records to
need to be deleted.

But now, in the common case where we migrate old data in the new table
and keep the ids, all of the associated data will be preserved.

Release Notes:

- Fixed a bug where workspace state would be lost when upgrading from
Zed 0.201.x. or below.
maxbrunsfeld added a commit that referenced this pull request Sep 8, 2025
Closes #37473

Previously, we enabled foreign keys at all times for our sqlite database
that we use for client-side state.
The problem with this is that In sqlite, `alter table` is somewhat
limited, so for many migrations, you must *recreate* the table: create a
new table called e.g. `workspace__2`, then copy all of the data from
`workspaces` into `workspace__2`, then delete the old `workspaces` table
and rename `workspaces__2` to `workspaces`. The way foreign keys work in
sqlite, when we delete the old table, all of its associated records in
other tables will be deleted due to `on delete cascade` clauses.

Unfortunately, one of the types of associated records that can be
deleted are `editors`, which sometimes store unsaved text. It is very
bad to delete these records, as they are the *only* place that this
unsaved text is stored.

This has already happened multiple times as we have migrated tables as
we develop Zed, but I caused it to happened again in
#36714.

The Sqlite docs recommend a multi-step approach to migrations where you:

* disable foreign keys
* start a transaction
* create a new table
* populate the new table with data from the old table
* delete the old table
* rename the new table to the old name
* run a foreign key check
* if it passes, commit the transaction
* enable foreign keys

In this PR, I've adjusted our sqlite migration code path to follow this
pattern more closely. Specifically, we disable foreign key checks before
running migrations, run a foreign key check before committing, and then
enable foreign key checks after the migrations are done.

In addition, I've added a generic query that we run *before* running the
foreign key check that explicitly deletes any rows that have dangling
foreign keys. This way, we avoid failing the migration (and breaking the
app) if a migration deletes data that *does* cause associated records to
need to be deleted.

But now, in the common case where we migrate old data in the new table
and keep the ids, all of the associated data will be preserved.

Release Notes:

- Fixed a bug where workspace state would be lost when upgrading from
Zed 0.201.x. or below.
maxbrunsfeld added a commit that referenced this pull request Sep 8, 2025
Closes #37473

Previously, we enabled foreign keys at all times for our sqlite database
that we use for client-side state.
The problem with this is that In sqlite, `alter table` is somewhat
limited, so for many migrations, you must *recreate* the table: create a
new table called e.g. `workspace__2`, then copy all of the data from
`workspaces` into `workspace__2`, then delete the old `workspaces` table
and rename `workspaces__2` to `workspaces`. The way foreign keys work in
sqlite, when we delete the old table, all of its associated records in
other tables will be deleted due to `on delete cascade` clauses.

Unfortunately, one of the types of associated records that can be
deleted are `editors`, which sometimes store unsaved text. It is very
bad to delete these records, as they are the *only* place that this
unsaved text is stored.

This has already happened multiple times as we have migrated tables as
we develop Zed, but I caused it to happened again in
#36714.

The Sqlite docs recommend a multi-step approach to migrations where you:

* disable foreign keys
* start a transaction
* create a new table
* populate the new table with data from the old table
* delete the old table
* rename the new table to the old name
* run a foreign key check
* if it passes, commit the transaction
* enable foreign keys

In this PR, I've adjusted our sqlite migration code path to follow this
pattern more closely. Specifically, we disable foreign key checks before
running migrations, run a foreign key check before committing, and then
enable foreign key checks after the migrations are done.

In addition, I've added a generic query that we run *before* running the
foreign key check that explicitly deletes any rows that have dangling
foreign keys. This way, we avoid failing the migration (and breaking the
app) if a migration deletes data that *does* cause associated records to
need to be deleted.

But now, in the common case where we migrate old data in the new table
and keep the ids, all of the associated data will be preserved.

Release Notes:

- Fixed a bug where workspace state would be lost when upgrading from
Zed 0.201.x. or below.
tidely pushed a commit to tidely/zed that referenced this pull request Sep 10, 2025
… other non-ssh remote projects (zed-industries#36714)

This is another pure refactor, to prepare for adding direct WSL support.

###  Todo

* [x] Represent `paths` in the same way for all workspaces, instead of
having a completely separate SSH representation
* [x] Adjust sqlite tables
    * [x] `ssh_projects` -> `ssh_connections` (drop paths)
    * [x] `workspaces.local_paths` -> `paths`
    * [x] remove duplicate path columns on `workspaces`
* [x] Add migrations for backward-compatibility

Release Notes:

- N/A

---------

Co-authored-by: Mikayla Maki <[email protected]>
tidely pushed a commit to tidely/zed that referenced this pull request Sep 10, 2025
tidely pushed a commit to tidely/zed that referenced this pull request Sep 10, 2025
This fixes a regression on nightly introduced in
zed-industries#36714

Release Notes:

- N/A
tidely pushed a commit to tidely/zed that referenced this pull request Sep 10, 2025
…s#37572)

Closes zed-industries#37473

### Background

Previously, we enabled foreign keys at all times for our sqlite database
that we use for client-side state.
The problem with this is that In sqlite, `alter table` is somewhat
limited, so for many migrations, you must *recreate* the table: create a
new table called e.g. `workspace__2`, then copy all of the data from
`workspaces` into `workspace__2`, then delete the old `workspaces` table
and rename `workspaces__2` to `workspaces`. The way foreign keys work in
sqlite, when we delete the old table, all of its associated records in
other tables will be deleted due to `on delete cascade` clauses.

Unfortunately, one of the types of associated records that can be
deleted are `editors`, which sometimes store unsaved text. It is very
bad to delete these records, as they are the *only* place that this
unsaved text is stored.

This has already happened multiple times as we have migrated tables as
we develop Zed, but I caused it to happened again in
zed-industries#36714.

### The Fix

The Sqlite docs recommend a multi-step approach to migrations where you:

* disable foreign keys
* start a transaction
* create a new table
* populate the new table with data from the old table
* delete the old table
* rename the new table to the old name
* run a foreign key check
* if it passes, commit the transaction
* enable foreign keys

In this PR, I've adjusted our sqlite migration code path to follow this
pattern more closely. Specifically, we disable foreign key checks before
running migrations, run a foreign key check before committing, and then
enable foreign key checks after the migrations are done.

In addition, I've added a generic query that we run *before* running the
foreign key check that explicitly deletes any rows that have dangling
foreign keys. This way, we avoid failing the migration (and breaking the
app) if a migration deletes data that *does* cause associated records to
need to be deleted.

But now, in the common case where we migrate old data in the new table
and keep the ids, all of the associated data will be preserved.

Release Notes:

- Fixed a bug where workspace state would be lost when upgrading from
Zed 0.201.x. or below.
FrGoIs pushed a commit to FrGoIs/zed that referenced this pull request Sep 29, 2025
… other non-ssh remote projects (zed-industries#36714)

This is another pure refactor, to prepare for adding direct WSL support.

###  Todo

* [x] Represent `paths` in the same way for all workspaces, instead of
having a completely separate SSH representation
* [x] Adjust sqlite tables
    * [x] `ssh_projects` -> `ssh_connections` (drop paths)
    * [x] `workspaces.local_paths` -> `paths`
    * [x] remove duplicate path columns on `workspaces`
* [x] Add migrations for backward-compatibility

Release Notes:

- N/A

---------

Co-authored-by: Mikayla Maki <[email protected]>
FrGoIs pushed a commit to FrGoIs/zed that referenced this pull request Sep 29, 2025
FrGoIs pushed a commit to FrGoIs/zed that referenced this pull request Sep 29, 2025
This fixes a regression on nightly introduced in
zed-industries#36714

Release Notes:

- N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed The user has signed the Contributor License Agreement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants