Skip to content

Fix RecycleCleanCommand critical database access bug and standardize logging#3854

Merged
jolestar merged 6 commits into
mainfrom
copilot/fix-recyclecleancommand-issues
Dec 9, 2025
Merged

Fix RecycleCleanCommand critical database access bug and standardize logging#3854
jolestar merged 6 commits into
mainfrom
copilot/fix-recyclecleancommand-issues

Conversation

Copilot AI commented Dec 9, 2025

Copy link
Copy Markdown
Contributor

RecycleCleanCommand Bug Fixes

✅ Phase 1: Critical Bug Fixes (COMPLETE)

  • Fix database access: Changed from open_rooch_db_readonly() to open_rooch_db() on line 574
    • Fixes runtime failure when attempting to delete data in read-only mode
  • Standardize logging: Replaced all println! and eprintln! with tracing macros
    • Consistent with codebase standards
    • Better structured logging for debugging and monitoring
    • Uses appropriate log levels (info, warn, error)
  • Use standard input handling: Replaced manual std::io operations with utils::read_line()
    • More robust error handling
    • Consistent with other commands
    • Maintains exact same safety barrier with "DELETE-RECYCLE-BIN" confirmation
  • Follow Rust conventions: Proper import organization at top of file
  • Fix rustfmt formatting: Applied cargo fmt to ensure code formatting passes
  • Code review: Passed with no issues
  • Security check: CodeQL analysis passed
  • Lint check: make lint formatting check passes

📝 Phase 2: Performance Optimization (Deferred to Follow-up PR)

The O(1) deletion optimization via column family drop requires architectural changes:

  • Would need to expose RocksDB instance to RecycleBinStore
  • Needs careful consideration of when to use O(1) vs O(N) strategy
  • Requires more extensive testing and validation
  • Will be addressed in a separate PR to keep changes minimal

🎯 Summary

This PR successfully fixes the three critical bugs identified:

  1. Database access bug (CRITICAL) - Command now works instead of failing
  2. Inconsistent logging - Now uses standard tracing macros
  3. Manual input handling - Now uses utility functions

All safety features are preserved:

  • --dry-run and --force requirements remain
  • User confirmation with "DELETE-RECYCLE-BIN" string maintained
  • Same safety barrier as original implementation

📊 Impact

  • ✅ RecycleCleanCommand now functions correctly
  • ✅ Consistent user experience with standardized logging
  • ✅ Better code maintainability
  • ✅ All safety features preserved
  • ✅ Code formatting passes make lint
Original prompt

This section details on the original issue you should resolve

<issue_title>commands/db: Fix RecycleCleanCommand critical bugs and performance issues</issue_title>
<issue_description>## 🐛 Bug Description

The RecycleCleanCommand has several critical issues that prevent it from functioning correctly and provide poor user experience:

1. Critical Database Access Bug

  • Issue: Uses open_rooch_db_readonly() instead of open_rooch_db() for deletion operations
  • Impact: Command fails at runtime when attempting to delete data in read-only database mode
  • Location: crates/rooch/src/commands/db/commands/recycle_bin.rs:574-579

2. Inconsistent Logging

  • Issue: Mixed usage of eprintln! and println! throughout the command
  • Impact: Poor user experience, inconsistent with codebase standards
  • Examples: Line 564 uses eprintln! for errors, Line 654 uses eprintln! for user prompts

3. Manual Input Handling

  • Issue: Uses manual std::io operations instead of existing utility functions
  • Impact: Poor error handling, inconsistent with other commands
  • Location: crates/rooch/src/commands/db/commands/recycle_bin.rs:654-663

4. Suboptimal Performance for Full Cleanup

  • Issue: Uses O(N) batch deletion even when deleting all entries
  • Impact: Inefficient when clearing the entire recycle bin
  • Opportunity: O(1) column family drop for full cleanup scenarios

🎯 Proposed Solution

Phase 1: Critical Bug Fixes

  1. Fix Database Access: Replace open_rooch_db_readonly() with open_rooch_db()
  2. Standardize Logging: Replace all print statements with structured tracing macros
  3. Use Standard Input Handling: Replace manual input with utils::prompt_yes_no()

Phase 2: Performance Optimization

  1. Hybrid Deletion Strategy:
    • O(1) deletion when no filters specified (drop column family)
    • O(N) deletion when selective filters are used (batch deletion)

📊 Performance Impact

Scenario Current Strategy Proposed Strategy Time Complexity
Delete all entries Batch deletion Column family drop O(N) → O(1)
Selective deletion Batch deletion Batch deletion O(N) (unchanged)

🛠 Implementation Details

Database Access Fix

// Current (BROKEN):
let (_root_meta, rooch_db, _start) = open_rooch_db_readonly(/*...*/);

// Fixed:
let (_root_meta, rooch_db, _start) = open_rooch_db(/*...*/);

Hybrid Deletion Strategy

pub fn delete_entries(&self, filter: &RecycleFilter, batch_size: usize) -> Result<usize> {
    // Check if this is a "delete all" operation
    if self.is_delete_all(filter) {
        tracing::info!("Deleting all recycle bin entries - using optimized column family drop");
        return self.drop_and_recreate_column_family();
    }
    
    // Use batch deletion for selective operations
    self.batch_delete_with_filter(filter, batch_size)
}

Standardized Logging

// Replace all println!/eprintln! with:
tracing::info!("User-facing messages");
tracing::error!("Error messages");
tracing::warn!("Warning messages");

Standard Input Handling

// Replace manual input with:
if !self.answer_yes && !utils::prompt_yes_no("Are you sure you want to permanently delete all entries matching the specified criteria?") {
    tracing::info!("Operation cancelled by user.");
    return Ok(None);
}

🧪 Testing Strategy

  1. Unit Tests: Test both deletion strategies with various filter combinations
  2. Integration Tests: Test with actual database operations
  3. Edge Cases: Empty recycle bin, different filter conditions
  4. Performance Tests: Verify O(1) vs O(N) behavior for different scenarios

🔒 Safety Considerations

  • Preserve existing safety checks: --dry-run and --force requirements remain
  • Maintain confirmation prompts: User approval still required for destructive operations
  • Add operation logging: Clear logging of which strategy is being used
  • Error handling: Proper error handling for column family operations

📝 Files to Modify

Primary Files

  • crates/rooch/src/commands/db/commands/recycle_bin.rs - Main implementation fixes
  • crates/rooch-pruner/src/recycle_bin.rs - Hybrid deletion strategy

Reference Files

  • crates/rooch/src/utils.rs - Utility functions to use
  • moveos/moveos-store/src/state_store/mod.rs - Column family operations

🎉 Expected Outcomes

  • Functional Command: RecycleCleanCommand will actually work instead of failing
  • Consistent User Experience: Standardized logging and input handling
  • Better Performance: O(1) deletion for full cleanup scenarios
  • Maintained Safety: All existing safety features preserved
  • Better Observability: Structured logging for debugging and monitoring

🏷 Labels

bug, enhancement, area::db, status::ready-for-dev

🔗 Related Is...


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@vercel

vercel Bot commented Dec 9, 2025

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rooch-portal-v2.1 Ready Ready Preview Comment Dec 9, 2025 3:21am
test-portal Ready Ready Preview Comment Dec 9, 2025 3:21am
1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
rooch Ignored Ignored Preview Dec 9, 2025 3:21am

- Fix database access: Changed from open_rooch_db_readonly() to open_rooch_db()
- Standardize logging: Replaced all println!/eprintln! with tracing macros
- Use standard input handling: Replaced manual input with utils::prompt_yes_no()

Co-authored-by: jolestar <77268+jolestar@users.noreply.github.com>
Copilot AI and others added 3 commits December 9, 2025 01:10
- Maintain high safety barrier by requiring explicit 'DELETE' confirmation
- Use utils::read_line() instead of manual std::io operations
- Add clearer warning messages using tracing

Co-authored-by: jolestar <77268+jolestar@users.noreply.github.com>
- Use 'DELETE-RECYCLE-BIN' instead of 'DELETE' to maintain the same safety level as original
- This ensures no reduction in safety barrier for this destructive operation

Co-authored-by: jolestar <77268+jolestar@users.noreply.github.com>
- Move read_line import to top with other imports
- Follows Rust conventions and improves code organization

Co-authored-by: jolestar <77268+jolestar@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix critical bugs and performance issues in RecycleCleanCommand Fix RecycleCleanCommand critical database access bug and standardize logging Dec 9, 2025
Copilot AI requested a review from jolestar December 9, 2025 01:16
@jolestar jolestar marked this pull request as ready for review December 9, 2025 01:53
Copilot AI review requested due to automatic review settings December 9, 2025 01:53
@github-actions

github-actions Bot commented Dec 9, 2025

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a critical runtime bug in RecycleCleanCommand and standardizes its logging to match codebase conventions. The command was previously attempting write operations (deletion) on a read-only database handle, causing runtime failures. Additionally, all console output has been migrated from println!/eprintln! to structured tracing macros for better log level control.

Key Changes:

  • Fixed database access mode from read-only to writable, enabling deletion operations
  • Standardized all logging using tracing macros with appropriate levels (info, warn, error)
  • Replaced manual input handling with the read_line() utility for consistency

@jolestar

jolestar commented Dec 9, 2025

Copy link
Copy Markdown
Contributor

@copilot 确保 make lint 可以通过

- Apply rustfmt formatting to recycle_bin.rs
- Fix line length for long tracing::warn! message
- Remove trailing whitespace
- Ensures make lint passes

Co-authored-by: jolestar <77268+jolestar@users.noreply.github.com>

Copilot AI commented Dec 9, 2025

Copy link
Copy Markdown
Contributor Author

@copilot 确保 make lint 可以通过

已修复。已应用 cargo fmt 确保代码格式符合标准。make lint 中的格式检查现在可以通过了。

Fixed in commit 8c64c89 - applied cargo fmt to ensure code formatting meets standards. The make lint formatting check now passes.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • esm.ubuntu.com
    • Triggering command: /usr/lib/apt/methods/https /usr/lib/apt/methods/https /home/REDACTED/.rustup/toolchains/1.82.0-x86_64-REDACTED-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libobject-a25e7f31e2204719.rlib /home/REDACTED/.rustup/toolchains/1.82.0-x86_64-REDACTED-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libmemchr-1f1858edbb50ccb5.rlib /home/REDACTED/.rustup/toolchains/1.82.0-x86_64-REDACTED-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libaddr2line-1319b75889377807.rlib /home/REDACTED/.rustup/toolchains/1.82.0-x86_64-REDACTED-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libgimli-c9972d847170eb93.rlib /home/REDACTED/.rustup/toolchains/1.82.0-x86_64-REDACTED-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/librustc_demangle-0868a75d0b80f801.rlib ipt_build-81472a11ec80f9bf.build_script_build.34e35a76a1578e34-cgu.0.rcgu.o ipt_build-81472a11ec80f9bf.build_script_build.34e35a76a1578e34-cgu.1.rcgu.o ipt_build-81472a11ec80f9bf.build_script_build.34e35a76a1578e34-cgu.2.rcgu.o ipt_build-81472a11ec80f9bf.build_script_build.34e35a76a1578e34-cgu.3.rcgu.o ipt_build-81472a11ec80f9bf.cj2ba1zxmi56yjp5qyclbbif0.rcgu.o /home/REDACTED/.rustup/toolchains/1.82.0-x86_64-REDACTED-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libunwind-d2b4a3bc57672e2c.rlib (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@jolestar jolestar merged commit 54f58ad into main Dec 9, 2025
26 of 28 checks passed
@jolestar jolestar deleted the copilot/fix-recyclecleancommand-issues branch December 9, 2025 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

commands/db: Fix RecycleCleanCommand critical bugs and performance issues

3 participants