Implement BLAKE3 Caching and Refactor Core Rclone Utilities#121
Implement BLAKE3 Caching and Refactor Core Rclone Utilities#121Amazing-Stardom merged 11 commits intomainfrom
Conversation
Implement XXHash Caching and Distributed LocksOverviewThis change introduces a persistent XXHash caching mechanism for local files. It implements a new distributed locking system for B2 storage. Core rclone operations and configuration initialization are significantly refactored. Technical Highlights
Impact
|
| SyncStatusDifferent = "*" | ||
| ) | ||
|
|
||
| // InitializeConfig sets up global configuration variables |
There was a problem hiding this comment.
Removed sync status constants. Clarify if these constants are replaced or if the sync status logic has been removed entirely.
| } | ||
|
|
||
| // Load config from b2m.toml | ||
| // Load config from b2m.toml |
| // Fetch user details | ||
| fetchUserDetails() | ||
|
|
||
| AppConfig.LocalDBDir = filepath.Join(AppConfig.ProjectRoot, "db", "all_dbs") |
There was a problem hiding this comment.
The path db/all_dbs is hardcoded. Consider making all_dbs a configurable constant if it might change.
There was a problem hiding this comment.
Will use in toml file
There was a problem hiding this comment.
This issue has been logged and will be investigated.
There was a problem hiding this comment.
That sounds like a good plan. Moving all_dbs to the toml file will make it configurable and addresses the concern about hardcoding.
There was a problem hiding this comment.
Thanks for the update. Using the TOML file for all_dbs will effectively address the hardcoding concern. Please ensure this change is implemented to make the path configurable.
| fetchUserDetails() | ||
|
|
||
| AppConfig.LocalDBDir = filepath.Join(AppConfig.ProjectRoot, "db", "all_dbs") | ||
| AppConfig.LocalVersionDir = filepath.Join(AppConfig.LocalDBDir, ".b2m", "version") |
There was a problem hiding this comment.
The new .b2m directory is introduced. Ensure this change is communicated to users regarding local file structure.
| return nil | ||
| } | ||
|
|
||
| func findProjectRoot() (string, error) { |
There was a problem hiding this comment.
The findProjectRoot function now searches for db or go.mod. This is a robust approach, but go.mod might not always indicate the project root for non-Go specific parts.
| } | ||
| parent := filepath.Dir(dir) | ||
| if parent == dir { | ||
| return "", fmt.Errorf("root not found (searched for 'db' dir or 'go.mod')") |
There was a problem hiding this comment.
The error message root not found is generic. Specify which of the two conditions (db dir or go.mod) failed.
| LogError("DownloadDatabase rclone copy failed for %s: %v", dbName, err) | ||
| return fmt.Errorf("download of %s failed: %w", dbName, err) | ||
| } | ||
| description := "Downloading " + dbName |
There was a problem hiding this comment.
The description parameter for RcloneCopy is hardcoded. Consider making it more dynamic or passing it from the caller for better context in logs.
| // 3.1. Calculate Local Hash of the newly downloaded file | ||
| localDBPath := filepath.Join(config.AppConfig.LocalDBDir, dbName) | ||
| localHash, err := CalculateSHA256(localDBPath) | ||
| localHash, err := CalculateXXHash(localDBPath) |
There was a problem hiding this comment.
Changed from CalculateSHA256 to CalculateXXHash. XXHash is faster but not cryptographically secure. Confirm if this change aligns with the security requirements for integrity checks.
| ) | ||
|
|
||
| // LockDatabase creates a .lock file | ||
| func LockDatabase(ctx context.Context, dbName, owner, host, intent string, force bool) error { |
There was a problem hiding this comment.
The force parameter for LockDatabase allows overriding existing locks. This could lead to data corruption if not used carefully.
| filename := fmt.Sprintf("%s.%s.%s.lock", dbName, owner, host) | ||
|
|
||
| // If forcing, we first clean up ALL existing locks for this DB to ensure we start fresh. | ||
| if force { |
There was a problem hiding this comment.
When force is true, all existing locks for the specific database are cleaned up. This scope (dbName.*.lock) is crucial and prevents accidental deletion of other database locks.
| } | ||
| defer os.Remove(tmpFile) | ||
|
|
||
| // Use RcloneCopy to upload the lock file |
There was a problem hiding this comment.
The FetchLocks followed by RcloneCopy is not an atomic operation. A race condition could occur where another client acquires a lock between the FetchLocks call and the RcloneCopy operation.
| } | ||
|
|
||
| // UnlockDatabase removes the .lock file | ||
| func UnlockDatabase(ctx context.Context, dbName, owner string, force bool) error { |
There was a problem hiding this comment.
The force parameter for UnlockDatabase allows deleting all locks for a database. This is powerful and potentially dangerous if misused.
| if force { | ||
| // Use rclone delete with include pattern | ||
| // Pattern: dbName.*.lock | ||
| pattern := fmt.Sprintf("%s.*.lock", dbName) |
There was a problem hiding this comment.
The pattern for rclone delete uses dbName.*.lock. If dbName itself contains wildcards, this could lead to unintended deletions.
|
|
||
| // cachedHash stores the hash and file stat info to avoid re-hashing unchanged files | ||
| type cachedHash struct { | ||
| Hash string |
There was a problem hiding this comment.
Consider using time.Time directly for ModTime instead of int64 (UnixNano). This improves type safety and readability.
| } | ||
|
|
||
| var ( | ||
| fileHashCache = make(map[string]cachedHash) |
There was a problem hiding this comment.
Global variables for cache and mutex can lead to tight coupling and make testing difficult.
| func CalculateXXHash(filePath string) (string, error) { | ||
| info, err := os.Stat(filePath) | ||
| if err != nil { | ||
| LogError("CalculateXXHash: Failed to stat file %s: %v", filePath, err) |
There was a problem hiding this comment.
The LogError call duplicates the error return. The caller will also receive the error.
|
|
||
| if ok && cached.ModTime == info.ModTime().UnixNano() && cached.Size == info.Size() { | ||
| return cached.Hash, nil | ||
| } else { |
There was a problem hiding this comment.
The else keyword is unnecessary here. The if block returns, so the following code is implicitly the else branch.
| if ok && cached.ModTime == info.ModTime().UnixNano() && cached.Size == info.Size() { | ||
| return cached.Hash, nil | ||
| } else { | ||
| LogInfo("Cache miss for %s. Cached: %v, Current: ModTime=%d, Size=%d", filepath.Base(filePath), ok, info.ModTime().UnixNano(), info.Size()) |
There was a problem hiding this comment.
Logging filepath.Base(filePath) might be insufficient for debugging if multiple files with the same base name exist in different directories.
| // Calculate hash | ||
| f, err := os.Open(filePath) | ||
| if err != nil { | ||
| LogError("CalculateXXHash: Failed to open file %s: %v", filePath, err) |
There was a problem hiding this comment.
The LogError call duplicates the error return. The caller will also receive the error.
| // Use streaming digest | ||
| h := xxh3.New() | ||
| if _, err := io.Copy(h, f); err != nil { | ||
| LogError("CalculateXXHash: io.Copy failed for %s: %v", filePath, err) |
There was a problem hiding this comment.
The LogError call duplicates the error return. The caller will also receive the error.
| } | ||
|
|
||
| // Sum64 returns uint64, format as hex string for compatibility | ||
| hash := fmt.Sprintf("%016x", h.Sum64()) |
There was a problem hiding this comment.
The hash is formatted as a 16-character hexadecimal string. Ensure this format is consistent with how hashes are stored and compared elsewhere in the system.
|
|
||
| data, err := os.ReadFile(cachePath) | ||
| if err != nil { | ||
| LogError("LoadHashCache: Failed to read cache file: %v", err) |
There was a problem hiding this comment.
The LogError call duplicates the error return. The caller will also receive the error.
| fileHashCacheMu.Lock() | ||
| defer fileHashCacheMu.Unlock() | ||
|
|
||
| if err := json.Unmarshal(data, &fileHashCache); err != nil { |
There was a problem hiding this comment.
The LogError call duplicates the error return. The caller will also receive the error.
|
|
||
| if err := json.Unmarshal(data, &fileHashCache); err != nil { | ||
| LogError("LoadHashCache: Failed to unmarshal cache: %v", err) | ||
| return fmt.Errorf("failed to unmarshal cache: %w", err) |
There was a problem hiding this comment.
Returning a wrapped error fmt.Errorf("failed to unmarshal cache: %w", err) is good practice.
| } | ||
|
|
||
| // Set version | ||
| model.AppConfig.ToolVersion = "2.0" |
There was a problem hiding this comment.
Severity: warning
The tool version is hardcoded here.
Suggestions:
- Consider making the
ToolVersionconfigurable via a build flag or a dedicated version file to avoid hardcoding.
| model.AppConfig.ToolVersion = "2.0" | ||
|
|
||
| // Load hash cache | ||
| if err := core.LoadHashCache(); err != nil { |
There was a problem hiding this comment.
Severity: info
Loading the hash cache early is good for performance, and handling a warning allows the application to proceed.
|
|
||
| case "--generate-hash": | ||
| // Dependencies check | ||
| if err := core.CheckB3SumAvailability(); err != nil { |
There was a problem hiding this comment.
Severity: warning
Dependency checks are duplicated here and later for normal startup.
Suggestions:
- Consider extracting common dependency checks into a single function to avoid duplication and ensure consistency.
| } | ||
|
|
||
| // Warning and Confirmation | ||
| fmt.Println("\nWARNING: This operation regenerates all metadata from local files.") |
There was a problem hiding this comment.
Severity: info
The warning and confirmation for --generate-hash is crucial for preventing accidental data loss.
| } | ||
|
|
||
| // Clean up .b2m before generating metadata | ||
| if err := core.CleanupLocalMetadata(); err != nil { |
There was a problem hiding this comment.
Severity: info
Cleaning up local metadata before generating new hashes is a correct step for this command.
| } | ||
|
|
||
| // Explicitly clear hash cache | ||
| core.ClearHashCache() |
There was a problem hiding this comment.
Severity: info
Explicitly clearing the hash cache is important when regenerating metadata.
| core.LogError("Startup Warning: %v", err) | ||
| } | ||
| core.HandleBatchMetadataGeneration() | ||
| Cleanup() |
There was a problem hiding this comment.
Severity: info
Calling Cleanup() ensures resources are properly released after a CLI command completes.
| case "--reset": | ||
| fmt.Println("Resetting system state...") | ||
| // Clean up .b2m before starting normal execution | ||
| if err := core.CleanupLocalMetadata(); err != nil { |
There was a problem hiding this comment.
Severity: info
Cleaning up local metadata for --reset is correct to ensure a fresh start.
| os.Exit(1) | ||
| } | ||
|
|
||
| // Explicitly clear hash cache |
There was a problem hiding this comment.
Severity: info
Clearing the hash cache for --reset ensures consistency.
| // Explicitly clear hash cache | ||
| core.ClearHashCache() | ||
|
|
||
| Cleanup() |
There was a problem hiding this comment.
Severity: info
Calling Cleanup() ensures resources are properly released after a CLI command completes.
| } | ||
|
|
||
| // Setup cancellation handling | ||
| sigHandler := core.NewSignalHandler() |
There was a problem hiding this comment.
Severity: info
Using NewSignalHandler encapsulates context management, improving modularity.
| core.LogError("Error: %v", err) | ||
| os.Exit(1) | ||
| } | ||
| if err := core.CheckRclone(); err != nil { |
There was a problem hiding this comment.
Severity: info
Logging a warning for rclone if it's not found suggests it might be optional for some features, but critical for others.
Suggestions:
- Clarify in documentation which features require
rclone.
| // fmt.Println("Warning: rclone not found or error:", err) | ||
| core.LogError("Warning: rclone not found or error: %v", err) | ||
| } | ||
| if !core.CheckRcloneConfig() { |
There was a problem hiding this comment.
Severity: info
Logging a warning for rclone config if it's not found suggests it might be optional for some features, but critical for others.
Suggestions:
- Clarify in documentation which features require
rclone.
|
|
||
| // Cleanup saves the hash cache and closes the logger. | ||
| // This should be called (usually deferred) by the main function. | ||
| func Cleanup() { |
There was a problem hiding this comment.
Severity: info
The Cleanup function provides a single point for graceful shutdown, ensuring resources like the hash cache and logger are properly closed.
| globalCancel context.CancelFunc | ||
| ) | ||
| // SignalHandler manages the application lifecycle context triggered by OS signals | ||
| type SignalHandler struct { |
There was a problem hiding this comment.
Severity: info
Encapsulating context and cancel function within a struct is a good pattern for managing lifecycle.
| type SignalHandler struct { | ||
| ctx context.Context | ||
| cancel context.CancelFunc | ||
| mu sync.Mutex |
There was a problem hiding this comment.
Severity: info
Using a sync.Mutex ensures thread-safe access to the context and cancel function.
| if globalCtx == nil { | ||
| SetupCancellation() | ||
| // Reset creates a new context watching for signals (clearing previous cancellation) | ||
| func (h *SignalHandler) Reset() { |
There was a problem hiding this comment.
Severity: info
The Reset() method correctly cancels the old context before creating a new one, preventing resource leaks.
| LogInfo("📝 Recording cancellation...") | ||
| meta, err := GenerateLocalMetadata(dbName, uploadDuration, "cancelled") | ||
|
|
||
| // strategy: Try to get existing remote metadata to preserve the "last valid state" |
There was a problem hiding this comment.
Severity: info
This comment block provides excellent context and reasoning for the refactor, highlighting a potential bug in previous logic.
| eventMeta, err := GenerateLocalMetadata(dbName, uploadDuration, "cancelled") | ||
| if err != nil { | ||
| LogError("⚠️ Failed to generate cancellation metadata: %v", err) | ||
| LogError("CleanupOnCancel: Failed to generate cancellation metadata for %s: %v", dbName, err) |
There was a problem hiding this comment.
Severity: critical
Robust error handling for GenerateLocalMetadata is crucial to ensure cancellation is recorded.
Suggestions:
- Return the error immediately to prevent further incorrect processing if metadata generation fails.
| ctx := context.Background() | ||
|
|
||
| // Try loading from Local Anchor (most reliable for "last known good state") | ||
| if localAnchor, err := GetLocalVersion(dbName); err == nil && localAnchor != nil { |
There was a problem hiding this comment.
Severity: info
Prioritizing local anchor metadata ensures the most recent known good state is used as a base.
| LogError("⚠️ Failed to append cancellation event: %v", err) | ||
| // Fallback: Try fetching single remote metadata | ||
| LogInfo("CleanupOnCancel: Local anchor missing for %s, trying remote fetch...", dbName) | ||
| remoteMeta, err := FetchSingleRemoteMetadata(ctx, dbName) |
There was a problem hiding this comment.
Severity: info
Falling back to remote metadata if local anchor is missing provides resilience.
| // 3. Construct the final metadata object | ||
| if baseMeta != nil { | ||
| // Use the base (remote/anchor) as the primary object to preserve Hash/Timestamp | ||
| meta = baseMeta |
There was a problem hiding this comment.
Severity: info
Setting meta = baseMeta is key to preserving the existing top-level metadata fields, preventing data loss.
| Status: "cancelled", | ||
| } | ||
| meta.Events = append(meta.Events, newEvent) | ||
| meta.Status = "cancelled" // Mark top-level status as cancelled to indicate last op failed |
There was a problem hiding this comment.
Severity: info
Explicitly setting meta.Status = "cancelled" at the top level clearly indicates the last operation's outcome.
| // Use the local info as the only record. | ||
| meta = eventMeta | ||
| // Ensure event #1 is present | ||
| if len(meta.Events) == 0 { |
There was a problem hiding this comment.
Severity: info
This conditional ensures that even if GenerateLocalMetadata somehow didn't produce an event, a 'cancelled' event is added for new files.
Suggestions:
- This is a defensive check; consider if
GenerateLocalMetadatashould guarantee at least one event.
| // 2. Download: Execute `rclone copy` to pull the file from B2. | ||
| // 3. Anchor: Construct a local "Verified Anchor" (LocalVersion) to mark this state as synced. | ||
| func DownloadDatabase(ctx context.Context, dbName string, onProgress func(model.RcloneProgress)) error { | ||
| func DownloadDatabase(ctx context.Context, dbName string, quiet bool, onProgress func(model.RcloneProgress)) error { |
There was a problem hiding this comment.
Severity: info
Adding a quiet parameter allows for more flexible control over download logging or progress display.
Suggestions:
- Ensure all call sites for
DownloadDatabaseare updated to pass the newquietparameter.
| filename := fmt.Sprintf("%s.%s.%s.%s", dbName, entry.Owner, entry.Hostname, entry.Type) | ||
|
|
||
| // Safety check: ensure we are only deleting a .lock file | ||
| if !strings.HasSuffix(filename, ".lock") { |
There was a problem hiding this comment.
Severity: warning
This safety check is redundant and potentially misleading. The entry.Type field, when properly parsed, should already ensure filename ends with .lock. The RcloneDeleteFile function should be responsible for ensuring operations occur within the LockDir and on valid lock files.
Suggestions:
- Remove this explicit
strings.HasSuffixcheck ifRcloneDeleteFileis robustly designed to only delete files within theLockDirand with expected lock file patterns. - Alternatively, strengthen
RcloneDeleteFileto enforce path and naming constraints, then this check becomes unnecessary.
| startTime := time.Now() | ||
|
|
||
| // Calculate hash using b3sum with timeout | ||
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) |
There was a problem hiding this comment.
Severity: warning
A 10-second timeout might be too short for very large files or systems with slow I/O, potentially leading to unnecessary failures.
Suggestions:
- Consider making the
b3sumtimeout configurable viamodel.AppConfig. - Increase the default timeout to a more conservative value (e.g., 30-60 seconds) to accommodate larger files.
| t.Render() | ||
| func sendDiscord(ctx context.Context, content string) { | ||
| payload := map[string]string{"content": content} | ||
| data, _ := json.Marshal(payload) |
There was a problem hiding this comment.
Severity: warning
json.Marshal can return an error; ignoring it might lead to curl sending malformed data if marshalling fails.
Suggestions:
- Check the error returned by
json.Marshaland handle it, e.g., by logging the error and returning early or sending a generic error message to Discord.
2 participants
Description
Fixes # (issue)
Type of Change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.
Screenshots (if applicable)
Add screenshots to help explain your changes.
Checklist
Additional Notes
Add any other context about the pull request here.