cleveragents/cleveragents-core

Fork 3

BUG-HUNT: [logic-error] LegacyDataMigrator renames JSON backup files outside the transaction — partial migration leaves corrupt state on failure #7764

New issue

Open

opened 2026-04-12 03:27:44 +00:00 by HAL9000 · 3 comments

HAL9000 commented

2026-04-12 03:27:44 +00:00

Owner

Bug Report: Logic Error — Backup File Rename Happens Outside Transaction in `LegacyDataMigrator`

Severity Assessment

Impact: If the transaction succeeds but the backup rename fails (e.g., disk full, permission error), the legacy JSON files are not renamed. On next startup, the migrator will attempt to re-migrate the same data, creating duplicate database records. If the rename partially succeeds (e.g., only plans.json is renamed before an error), the state is inconsistent.
Likelihood: Low-Medium — file rename after a successful DB transaction can fail independently
Priority: Medium

Location

File: src/cleveragents/infrastructure/database/legacy_migrator.py
Function/Class: LegacyDataMigrator.migrate_project_data
Lines: 235–244

Description

The legacy migrator follows a two-phase approach:

Migrate data to SQLite inside with self.unit_of_work.transaction() as ctx: (lines 65–232)
Rename source JSON files to .backup AFTER the transaction commits (lines 235–240)

The file rename operations are outside the with block — they happen after the transaction has already committed successfully. If any of these renames fail, the DB data is persisted but the JSON files are NOT marked as migrated. The next startup will attempt to re-migrate the same data, causing IntegrityError or creating duplicate records.

Additionally, if plans.json is successfully renamed but contexts.json rename fails, the state is partially migrated — next startup won't find plans.json and may skip plan migration while still trying to migrate contexts.

Evidence

# legacy_migrator.py lines 64-244
with self.unit_of_work.transaction() as ctx:  # <-- transaction block
    # ... migrate plans, contexts, changes to DB ...
    pass  # transaction committed here

# OUTSIDE the transaction:
if plans_json.exists():
    plans_json.rename(plans_json.with_suffix(".json.backup"))  # can fail!
if contexts_json.exists():
    contexts_json.rename(contexts_json.with_suffix(".json.backup"))  # can fail!
if changes_json.exists():
    changes_json.rename(changes_json.with_suffix(".json.backup"))  # can fail!

return True

The outer except (json.JSONDecodeError, OSError) at line 243 catches file errors but only returns False — which gives the false impression that no migration occurred, when in reality the DB was already updated.

Expected Behavior

After a successful database migration:

If any file rename fails, it should be logged as a warning but not cause re-migration on next startup (perhaps by writing a .migrated sentinel file)
OR the migration guard should check both the DB AND the sentinel, not just the JSON file presence

Actual Behavior

File renames after the transaction commit can fail silently (caught by the broad except OSError), causing the migrator to return False even though data was successfully migrated. Next startup will attempt re-migration.

Suggested Fix

Write a migration sentinel file inside the transaction scope, or handle the post-commit rename errors separately from the migration logic:

# After transaction succeeds, handle renames with proper error handling:
for json_file in [plans_json, contexts_json, changes_json]:
    if json_file.exists():
        try:
            json_file.rename(json_file.with_suffix(".json.backup"))
        except OSError as e:
            # Log warning but don't fail -- data is already in DB
            logging.warning("Could not rename %s: %s", json_file, e)

return True

Separate the OSError catching for file renames from the json.JSONDecodeError catching for parse errors.

TDD Note

After this bug issue is verified, a corresponding Type/Testing issue will be created for TDD.

Automated by CleverAgents Bot
Supervisor: Bug Hunting | Agent: bug-hunter

## Bug Report: Logic Error — Backup File Rename Happens Outside Transaction in `LegacyDataMigrator` ### Severity Assessment - **Impact**: If the transaction succeeds but the backup rename fails (e.g., disk full, permission error), the legacy JSON files are not renamed. On next startup, the migrator will attempt to re-migrate the same data, creating duplicate database records. If the rename partially succeeds (e.g., only `plans.json` is renamed before an error), the state is inconsistent. - **Likelihood**: Low-Medium — file rename after a successful DB transaction can fail independently - **Priority**: Medium ### Location - **File**: `src/cleveragents/infrastructure/database/legacy_migrator.py` - **Function/Class**: `LegacyDataMigrator.migrate_project_data` - **Lines**: 235–244 ### Description The legacy migrator follows a two-phase approach: 1. Migrate data to SQLite inside `with self.unit_of_work.transaction() as ctx:` (lines 65–232) 2. Rename source JSON files to `.backup` AFTER the transaction commits (lines 235–240) The file rename operations are outside the `with` block — they happen after the transaction has already committed successfully. If any of these renames fail, the DB data is persisted but the JSON files are NOT marked as migrated. The next startup will attempt to re-migrate the same data, causing `IntegrityError` or creating duplicate records. Additionally, if `plans.json` is successfully renamed but `contexts.json` rename fails, the state is partially migrated — next startup won't find `plans.json` and may skip plan migration while still trying to migrate contexts. ### Evidence ```python # legacy_migrator.py lines 64-244 with self.unit_of_work.transaction() as ctx: # <-- transaction block # ... migrate plans, contexts, changes to DB ... pass # transaction committed here # OUTSIDE the transaction: if plans_json.exists(): plans_json.rename(plans_json.with_suffix(".json.backup")) # can fail! if contexts_json.exists(): contexts_json.rename(contexts_json.with_suffix(".json.backup")) # can fail! if changes_json.exists(): changes_json.rename(changes_json.with_suffix(".json.backup")) # can fail! return True ``` The outer `except (json.JSONDecodeError, OSError)` at line 243 catches file errors but only returns `False` — which gives the false impression that no migration occurred, when in reality the DB was already updated. ### Expected Behavior After a successful database migration: - If any file rename fails, it should be logged as a warning but not cause re-migration on next startup (perhaps by writing a `.migrated` sentinel file) - OR the migration guard should check both the DB AND the sentinel, not just the JSON file presence ### Actual Behavior File renames after the transaction commit can fail silently (caught by the broad `except OSError`), causing the migrator to return `False` even though data was successfully migrated. Next startup will attempt re-migration. ### Suggested Fix Write a migration sentinel file inside the transaction scope, or handle the post-commit rename errors separately from the migration logic: ```python # After transaction succeeds, handle renames with proper error handling: for json_file in [plans_json, contexts_json, changes_json]: if json_file.exists(): try: json_file.rename(json_file.with_suffix(".json.backup")) except OSError as e: # Log warning but don't fail -- data is already in DB logging.warning("Could not rename %s: %s", json_file, e) return True ``` Separate the `OSError` catching for file renames from the `json.JSONDecodeError` catching for parse errors. ### Category logic-error ### TDD Note After this bug issue is verified, a corresponding Type/Testing issue will be created for TDD. --- **Automated by CleverAgents Bot** Supervisor: Bug Hunting | Agent: bug-hunter

HAL9000 added this to the v3.2.0 milestone

2026-04-12 03:45:55 +00:00

HAL9000 referenced this issue

2026-04-12 03:47:56 +00:00

[AUTO-PROJ-OWN] Project Owner Report (Cycle 1) #7724

HAL9000 added the

labels

2026-04-12 05:47:58 +00:00

freemo added the

Type

Bug

label

2026-04-12 18:33:40 +00:00

HAL9000 added

and removed

labels

2026-04-14 05:58:22 +00:00

HAL9000 commented

2026-04-14 05:58:22 +00:00

Author

Owner

✅ Verified — Logic error: LegacyDataMigrator renames files outside transaction — partial migration leaves corrupt state. MoSCoW: Must-have. Priority: High — data integrity risk.

Automated by CleverAgents Bot
Supervisor: Project Owner | Agent: project-owner-pool-supervisor

✅ **Verified** — Logic error: LegacyDataMigrator renames files outside transaction — partial migration leaves corrupt state. MoSCoW: Must-have. Priority: High — data integrity risk. --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: project-owner-pool-supervisor

HAL9000 commented

2026-04-14 06:11:39 +00:00

Author

Owner

✅ Verified — Logic error: LegacyDataMigrator renames files outside transaction — partial migration leaves corrupt state. MoSCoW: Must-have. Priority: High — data integrity risk.

Automated by CleverAgents Bot
Supervisor: Project Owner | Agent: project-owner-pool-supervisor

HAL9000 commented

2026-04-14 06:15:08 +00:00

Author

Owner

✅ Verified — Logic error: LegacyDataMigrator renames files outside transaction — partial migration leaves corrupt state. MoSCoW: Must-have. Priority: High — data integrity risk.

Automated by CleverAgents Bot
Supervisor: Project Owner | Agent: project-owner-pool-supervisor