Skip to content

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

Closed
immakermatty wants to merge 1 commit into
pingdotgg:mainfrom
Lazurio:fix/v2-import-leftover-dirs
Closed

immakermatty wants to merge 1 commit into
pingdotgg:mainfrom
Lazurio:fix/v2-import-leftover-dirs

Conversation

@immakermatty

@immakermatty immakermatty commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

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

…y on disk

The first V2 start copies state.sqlite into a .v2-import-* directory
before it publishes the copy. A start killed during the copy never
removed that directory, and nothing else did, so every killed attempt
left a database-sized directory in userdata.

The directory name now carries the copying process ID. Every start
removes .v2-import-* directories whose process no longer runs, and the
ones from older builds, which carry no ID.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Codex Review Summary

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

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-19T22:16:18.026111Z 3c28f00 PR 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.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 19, 2026
@coderabbitai

coderabbitai Bot commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

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 a71a6c3 and 3c28f00.

📒 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 name Status Explanation
Title check Passed The 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 check Passed 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…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check 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.

Copy link
Copy Markdown
Collaborator

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 #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.

@maria-rcks maria-rcks closed this Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants