Repository navigation
fix(core): repair V1 global sessions left by completed imports - #58846
rekram1-node wants to merge 1 commit into
Conversation
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The repair works. I built an already-upgraded database (import marked completed, V1 global sessions in an existing folder and in a missing folder, plus a V2-native global session) and ran serve plus session list from each checkout. On the base the folder's session list was empty. With this PR the V1 session moves to the folder's project with time_updated kept and the repair is marked done. The missing-folder and V2-native sessions stay under global. v1-migration tests pass (the new one fails on the base) and the packages/core type check passes.
Public API: unchanged. The only addition is an internal kv key, migration.v1-v2.global-sessions.
One thing worth fixing (inline): after a lookup timeout the repair still marks itself done, so the remaining folders are never repaired.
Related: #57067
| updateProgress({ label: "Repairing V1 sessions", numerator: index, denominator: folders.length }) | ||
| const resolved = yield* resolveDirectory(folder.directory) | ||
| // One hung folder (e.g. a dead network mount) usually means more; leave the rest under global. | ||
| if (Option.isNone(resolved)) { |
There was a problem hiding this comment.
After a timeout this breaks and then still writes REPAIR_STATE_KEY as completed below. So every folder after the hung one stays under global for good, even healthy local ones. Folders are processed in alphabetical order, so one dead mount early in the list can block many real projects. This also differs from the description ("the next start does the rest").
Two possible fixes:
returnwithout marking done on timeout. The next start retries, and since it runs in the background a dead mount only costs ~5s per start.- Skip just that folder and continue.
The import can afford to give up after a timeout because it runs once anyway. This repair is the only chance to fix these sessions.
matt8s
left a comment
matt8s
left a comment
There was a problem hiding this comment.
Two unfinished cases still receive the repair-completed marker:
- In the completed-import repair, the timeout
breakreaches the marker write below it. Returning instead preserves earlier folder commits and lets the next run retry the remainder. - An initial import can time out, or resume past already-copied global sessions, and still mark repair complete. Letting the follow-up repair alone write its completion marker covers both cases.
The follow-up commits are 14fa58c and fa9e374, on the tested branch. Beyond this PR, the production change is one early return and removal of the importer's repair-marker write.
The eight-case synthetic regression covers completed, fresh, and resumed imports, timeouts at the first and second lookup, healthy completion, and retry/idempotence. The tests check parent-child links, timestamps, transcripts, committed ownership changes, and ownership exclusions. The new cases fail without the fixes; all 450 repeated migration tests pass with them.
On the branch merged with current dev, the full Core suite passed: 7,064 passed, 25 skipped, zero failures, with an isolated OAuth callback port. Repository-root bun run check passed all 35 tasks; Core's pinned TypeScript check and changed-file formatting also passed.
Could these follow-ups be incorporated here? The existing repair and its author attribution are preserved; this avoids opening a competing implementation.
|
Tested the repair logic against a real already-migrated DB, not a synthetic one (v2.0.32 import, 17 GB, 5,241
Two gaps/questions beyond the timeout marker (already flagged by the bot and @matt8s — +1 for
If useful, I can run this branch against a copy of the DB and report concrete before/after counts ( |
|
Here is a patch on top of New test coverage:
Verified locally on The code part overlaps with @matt8s's commits, so treat this as one consolidated diff rather than a competing fix. Apply with Patchdiff --git a/packages/core/src/database/v1-migration.bun.ts b/packages/core/src/database/v1-migration.bun.ts
index 760aa98b17..d5faf088bd 100644
--- a/packages/core/src/database/v1-migration.bun.ts
+++ b/packages/core/src/database/v1-migration.bun.ts
@@ -762,12 +762,6 @@ export function run(
set: { value: { phase: "completed" }, time_updated: Date.now() },
})
.run()
- // This import already attributed global sessions to their folders' projects.
- yield* tx
- .insert(KVTable)
- .values({ key: REPAIR_STATE_KEY, value: { phase: "completed" } })
- .onConflictDoNothing()
- .run()
}),
)
.pipe(Effect.orDie)
@@ -792,12 +786,13 @@ function repairGlobalSessions(db: Database.Interface["db"]) {
for (const [index, folder] of folders.entries()) {
updateProgress({ label: "Repairing V1 sessions", numerator: index, denominator: folders.length })
const resolved = yield* resolveDirectory(folder.directory)
- // One hung folder (e.g. a dead network mount) usually means more; leave the rest under global.
+ // One hung folder (e.g. a dead network mount) usually means more; leave the rest under global
+ // and retry on the next start instead of marking the repair complete.
if (Option.isNone(resolved)) {
yield* Effect.logWarning("Stopped repairing V1 global sessions after a timeout", {
directory: folder.directory,
})
- break
+ return
}
const owner = resolved.value
if (!owner || owner.project.id === Project.ID.global) continue
diff --git a/packages/core/test/v1-migration.test.ts b/packages/core/test/v1-migration.test.ts
index af05eb16e1..423e8831b6 100644
--- a/packages/core/test/v1-migration.test.ts
+++ b/packages/core/test/v1-migration.test.ts
@@ -1263,6 +1263,8 @@ describe("V1Migration database workflow", () => {
await fs.mkdir(plain, { recursive: true })
await fs.mkdir(path.join(repo, "pkg", "a"), { recursive: true })
await initRepo(repo)
+ const linked = path.join(tmp.path, "plain-link")
+ await fs.symlink(plain, linked, process.platform === "win32" ? "junction" : undefined)
await database(
Effect.gen(function* () {
const { db } = yield* Database.Service
@@ -1274,21 +1276,22 @@ describe("V1Migration database workflow", () => {
sql`INSERT INTO kv (key, value, time_created, time_updated) VALUES ('migration.v1-v2', '{"phase":"completed"}', 1, 1)`,
)
const sessions = [
- { id: "ses_plain", directory: plain, v1: true },
- { id: "ses_repo", directory: path.join(repo, "pkg", "a"), v1: true },
- { id: "ses_missing", directory: missing, v1: true },
- { id: "ses_native", directory: plain, v1: false },
+ { id: "ses_plain", directory: plain, path: "legacy/plain", v1: true },
+ { id: "ses_linked", directory: linked, path: "legacy/linked", v1: true },
+ { id: "ses_repo", directory: path.join(repo, "pkg", "a"), path: "legacy/repo", v1: true },
+ { id: "ses_missing", directory: missing, path: "legacy/missing", v1: true },
+ { id: "ses_native", directory: plain, path: null, v1: false },
]
yield* Effect.forEach(sessions, (input) =>
Effect.gen(function* () {
if (input.v1)
yield* db.run(sql`
- INSERT INTO session (id, project_id, slug, directory, title, version, time_created, time_updated)
- VALUES (${input.id}, 'global', ${input.id}, ${input.directory}, 'Test', '1', 1, 2)
+ INSERT INTO session (id, project_id, slug, directory, path, title, version, time_created, time_updated)
+ VALUES (${input.id}, 'global', ${input.id}, ${input.directory}, ${input.path}, 'Test', '1', 1, 2)
`)
yield* db.run(sql`
- INSERT INTO session_v2 (id, project_id, slug, directory, title, version, time_created, time_updated)
- VALUES (${input.id}, 'global', ${input.id}, ${input.directory}, 'Test', '1', 1, 2)
+ INSERT INTO session_v2 (id, project_id, slug, directory, path, title, version, time_created, time_updated)
+ VALUES (${input.id}, 'global', ${input.id}, ${input.directory}, ${input.path}, 'Test', '1', 1, 2)
`)
}),
)
@@ -1298,12 +1301,16 @@ describe("V1Migration database workflow", () => {
plain: yield* projects.resolve(AbsolutePath.make(plain)),
repo: yield* projects.resolve(AbsolutePath.make(repo)),
}
- expect(yield* db.all(sql`SELECT id, project_id, path, time_updated FROM session_v2 ORDER BY id`)).toEqual([
- { id: "ses_missing", project_id: "global", path: null, time_updated: 2 },
+ const repaired = [
+ { id: "ses_linked", project_id: owners.plain.id, path: "", time_updated: 2 },
+ { id: "ses_missing", project_id: "global", path: "legacy/missing", time_updated: 2 },
{ id: "ses_native", project_id: "global", path: null, time_updated: 2 },
{ id: "ses_plain", project_id: owners.plain.id, path: "", time_updated: 2 },
{ id: "ses_repo", project_id: owners.repo.id, path: "pkg/a", time_updated: 2 },
- ])
+ ]
+ expect(yield* db.all(sql`SELECT id, project_id, path, time_updated FROM session_v2 ORDER BY id`)).toEqual(repaired)
+ expect(yield* V1Migration.run()).toEqual({ status: "completed" })
+ expect(yield* db.all(sql`SELECT id, project_id, path, time_updated FROM session_v2 ORDER BY id`)).toEqual(repaired)
expect(yield* db.get(sql`SELECT value FROM kv WHERE key = 'migration.v1-v2.global-sessions'`)).toEqual({
value: '{"phase":"completed"}',
}) |
|
@jimicze I’ll incorporate your complementary legacy-path and canonical-alias coverage into the existing follow-up branch, retaining its timeout, parent/child, transcript and timestamp tests. The two production changes in your patch are already in that branch, so no competing repair implementation is needed. The non-timeout retry gap is also present: an unresolved owner is skipped, then the repair writes its completed marker. I’ll add regression coverage and withhold that marker when any eligible folder remains unresolved, while continuing to repair healthy folders. This also retries missing directories on a later startup if they return; no background polling is added. @rekram1-node I’m retaining the existing workspace and legacy-row eligibility safeguards. Is that still the intended scope? Expanding repair to explicit workspaces or rows without proof of V1 origin needs a separate placement/provenance rule. I’ll report validation against current |
|
The follow-up is implemented in f021b04, atop the existing timeout fixes. It incorporates @jimicze’s legacy-path and alias coverage while retaining the NULL-path, parent/child, transcript and timestamp checks. Unresolved eligible folders now withhold the repair-completed marker without blocking healthy folders in the same pass. Tests cover permission errors, failed directory checks, project-resolution defects, and a missing folder that returns on a later startup. Workspace and legacy-provenance exclusions are retained. The existing stop-on-timeout policy is unchanged; no API, schema, dependency or polling worker is added. Hosted validation on integration commit
The source is on the existing repair-timeout branch. Could the follow-up commits be incorporated here? They preserve this PR’s original history and attribution. These results cover the stated revisions, not a newer upstream merge or the skipped tests. Update: after three more upstream commits landed, integration |
Issue for this PR
Fixes #49827 for people who already upgraded.
Type of change
What does this PR do?
#56322 attributes V1 global sessions to their folder's project during the import, but only for imports that hadn't finished. Anyone who already upgraded still has those sessions under
global, where the per-folder session lists can't find them.This adds a one-time repair that runs in the background on server start, after the import:
session_v2rows that are stillglobal, have no workspace, and also exist in the V1sessiontable. Sessions created in V2 are left alone.sessiontable still has the originalglobalproject ids.Also points the orphan-session test at a missing folder inside its own temp dir instead of
/tmp/orphan, which could exist on a machine.How did you verify your code works?
bun run checkpasses;v1-migrationandprojecttests pass with--rerun-each 3.Checklist
Added by the opencode bot (AI-generated).
Related: #57067