Sign up
View stargazersSign up

fix(server): a start killed while copying V1 no longer leaves its copy on disk#21964

closed1
main
GitHub fork: fix/v2-import-leftover-dirs
opened11d agobyIimmakermatty
2 files changed
View on GitHubTwo-way sync enabledPR last imported 5d ago

Problem

The first 0.1.x start copies state.sqlite into a .v2-import-* directory in userdata before it publishes the copy. A normal failure removes the directory, but a start killed during the copy (SIGKILL, a crash, power loss) leaves it behind, and nothing removes it later. It is as big as the V1 database, and every killed attempt adds another one.

Change

  • The directory name now carries the copying process ID: .v2-import-<pid>-XXXXXX.
  • Every start removes .v2-import-* directories whose process no longer runs, using isProcessAlive from serverRuntimeState.ts, and the ones from older builds, which carry no ID.
  • The directory of a copy that is still running is kept. A failed cleanup is logged and never blocks the start.

Limits:

  • An older build copying in the same userdata at the same moment would lose its directory. That start fails with the existing import error and copies again next time.
  • If the process ID has been reused, the leftover stays until that process exits.

Scope and approval

A small, focused fix for an obvious leak: a temporary directory that nothing removes. It sits on the same import path as #15017, but it's a separate defect, so it's not in #21965.

Verification

  • A new test in initializeV2Database.test.ts: a directory from a process that has exited and one from an older build are removed, and one owned by the running process stays. It fails without the cleanup, when the cleanup ignores whether the process runs, and when it spares directories from older builds.
  • End to end with the real import: a 413 MB V1 database, with the importing process SIGKILLed about 50 MB into the copy, then a normal start.
    • main: the 50 MB .v2-import-* directory is still there after the next start.
    • This branch: the next start removes it and publishes the copy. A leftover from main's code, which has no process ID, is removed on the following start.
  • vp test run apps/server/src/persistence/initializeV2Database.test.ts (5 passed), vp run --filter t3 typecheck and vp lint on the changed files pass.

Not checked: Windows. isProcessAlive already relies on process.kill(pid, 0) there.

Model: Claude Opus 5.5. Harness: Claude Code inside T3 Code.

🤖 Generated with Claude Code

Checks

6 successful · 10 skipped
All checks passed
3c28f00

Added 1 commit

  • fix(server): a start killed while copying V1 no longer leaves its copy on disk3c28f00
Cchatgpt-codex-connector[bot]11d ago
Synced from GitHub

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

ReviewStatusCommitReview trigger
📝 Code Review✅ Completed 2026-10-19T22:16:18.026111Z3c28f00PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

CcoderabbitaiBot11d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 2202e4ad-e6cf-44b1-8132-418b64ba2edf
📥 Commits

Reviewing files that changed from the base of the PR and between a71a6c37d14154c8cb355242d569e8a1dbeec706 and 3c28f00b40a24aae3c36ac26c79c89a655079b52.

📒 Files selected for processing: 2
  • apps/server/src/persistence/initializeV2Database.test.ts
  • apps/server/src/persistence/initializeV2Database.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Initialization now removes abandoned V2 import-copy directories before its existing destination and source checks. It keeps directories linked to live processes and includes the current process ID in the prefix for new temporary directories.

Changes: V2 import-copy cleanup

Layer / File(s)Summary
Clean up abandoned import copies
apps/server/src/persistence/initializeV2Database.ts, apps/server/src/persistence/initializeV2Database.test.ts		Initialization removes matching directories with no process ID or a process that is not alive. It retains directories associated with a live process, ignores cleanup errors, and names new temporary directories with the current process ID. The test covers exited, unknown, and current process IDs.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 3c28f

This change removes leftover partial V1 database copies after a killed start, and cleanup failures do not block startup. The remaining risks are narrow, and no merge-blocking issue was found.

### Architecture Summary

Architecture risk: 🔵 Low · up to 3c28f

The change affects 1 system.

Changed systems: apps/server

Architecture concerns No architecture-level concerns identified.

Review details

Systems and components

  • observed — apps/server (service) was modified; 2 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in apps/server/src/persistence/initializeV2Database.test.ts: The test file adds the node:child_process import.
  • observed — Modified behavior in apps/server/src/persistence/initializeV2Database.test.ts: Adds a test that creates partial-copy directories associated with an exited process, an unknown process ID, and the current process; after initialization, it expects only the current-process directory to remain.
  • observed — Modified behavior in apps/server/src/persistence/initializeV2Database.ts: Imports isProcessAlive for checking whether a process associated with an import directory is still running.
  • observed — Modified behavior in apps/server/src/persistence/initializeV2Database.ts: Adds removeAbandonedCopies, which reads the directory and treats a not-found error as an empty listing. It recursively removes entries matching .v2-import- when their name has no process ID or the named process is not alive; entries with a live process ID and names that do not match are retained.
Pre-merge checks: Passed 4
Check nameStatusExplanation
Title checkPassedThe title is concise, uses the repository's conventional format, and clearly describes the fix for leftover V1 copy directories after a killed server start.
Description checkPassedThe description includes the problem, change, scope rationale, limitations, focused tests, end-to-end verification, reported commands, and the untested Windows case. It satisfies the required template…
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description includes the problem, change, scope rationale, limitations, focused tests, end-to-end verification, reported commands, and the untested Windows case. It satisfies the required template.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Mmaria5d ago

Note

Written by gpt-6.1-sol on behalf of Maria

thanks @immakermatty for the pid-aware cleanup, live-process guard, legacy-copy handling, and regression coverage. those address the leak directly.

i tried updating your branch with main merged in, but github rejected the push to Lazurio/t3code with a 403 despite maintainerCanModify: true, so i rebuilt it in https://github.com/pingdotgg/t3code/pull/25301. it covers the same abandoned-copy leak and credits you in the commit.

i compared the original diff against the replacement. the startup cleanup, pid-prefixed names, live-process guard, legacy cleanup, and nonblocking error handling remain. the pid probe moved into the persistence module to avoid a circular import through startup/authentication/sqlite. the filename match is narrower, the explanatory comment is shorter, and the existing regression also checks unrelated entries and existing v2 data. no other behavior was dropped.