added upload caching

[LinoGiger]

No description provided.

[claude]

PR Review: Upload Caching Feature

Summary

This PR adds caching functionality to prevent re-uploading assets that have already been uploaded. The implementation includes a configuration flag and a cache mechanism based on file metadata.

---

Code Quality & Best Practices

Positive aspects:

• Clean implementation with a dedicated cache key generation method
• Proper use of type hints (dict[str, str])
• Configuration-driven approach with cache_uploads flag (defaults to True)
• Good logging at appropriate levels (debug for cache hits, info for uploads)

Areas for improvement:

1. Cache Key Generation for URLs (asset_uploader.py:22-23)

   • URL cache keys could collide if the URL content changes
   • Consider adding URL validation or content hashing for more robust caching

   # Current: URLs are cached indefinitely based on the URL string alone
   if re.match(r"^https?://", asset):
       return asset

2. Error Handling (asset_uploader.py:25-27)

   • os.stat() can raise FileNotFoundError or PermissionError but these aren't caught
   • This happens before the existing FileNotFoundError check at line 44, creating inconsistent error handling
   • Recommendation: Wrap _get_cache_key() call in try-except or move file existence check earlier

3. f-string Consistency (asset_uploader.py:49)

   • Mixed use of f-string formatting (logger.info(f"Asset uploaded: {response.file_name}")) vs % formatting elsewhere
   • Recommend sticking to % formatting for consistency with existing code (lines 31, 36, 52)

---

Potential Bugs & Issues

Critical Issues:

1. Cache Lifetime & Memory Leak Risk

   • The _upload_cache dictionary grows unbounded during the lifetime of an AssetUploader instance
   • In long-running applications or batch processing scenarios, this could consume significant memory
   • Impact: If processing thousands of unique assets, cache will retain all keys indefinitely
   • Recommendation: Consider implementing:
     • LRU cache with size limit (e.g., using functools.lru_cache decorator or collections.OrderedDict)
     • TTL-based eviction
     • Persistent cache (disk-based) for cross-session benefits
     • Documentation on cache scope and lifetime

2. Cache Key Race Condition

   • File modification time (st_mtime_ns) can change between cache key generation and actual upload
   • Could result in uploading an old version but returning a new cache key
   • Likelihood: Low, but possible in scenarios with file watching/auto-save systems
   • Recommendation: Generate cache key after successful upload or use file content hash instead

3. Cache Configuration Check Placement (asset_uploader.py:50-51)

   • Cache is populated after upload completes, but cache check happens before upload
   • This is correct, but the conditional check means:
     • If cache_uploads=False, cache is never populated but could still be checked (no harm currently)
     • Changing config mid-execution could lead to inconsistent behavior
   • Recommendation: Either check config flag in both places or document config immutability

---

Performance Considerations

Positive:

• Significant performance improvement for repeated assets (avoids redundant network uploads)
• O(1) cache lookups using dict

Considerations:

1. File Stat System Call (asset_uploader.py:25)

   • os.stat() is called on every upload attempt, even for cache hits
   • For cache hits, this is redundant (we only need the cached result)
   • Impact: Minimal for local files, but could add latency for networked file systems
   • Recommendation: Consider restructuring to check cache key generation after URL check:

   if re.match(r"^https?://", asset):
       cache_key = asset
       if cache_key in self._upload_cache:
           return self._upload_cache[cache_key]
       # ... upload URL
   else:
       cache_key = self._get_cache_key(asset)  # Only stat() if not URL
       if cache_key in self._upload_cache:
           return self._upload_cache[cache_key]
       # ... upload file

2. Cache Invalidation Strategy

   • No mechanism to invalidate cache entries if remote storage state changes
   • If an asset is deleted from the server, cached references become stale
   • Consider documenting this limitation or adding optional cache clearing methods

---

Security Concerns

1. Path Traversal (Low risk)

   • Using file paths directly in cache keys could expose internal directory structure in logs
   • Cache keys include full file paths (asset_uploader.py:27)
   • Recommendation: Consider hashing the path or using relative paths for cache keys

2. Timing Attacks (Very low risk)

   • Using st_mtime_ns ties cache to filesystem metadata which could leak timing info
   • Unlikely to be exploitable in this context but worth noting
   • Alternative: Use content-based hashing (e.g., SHA256 of file)

---

Test Coverage

Missing test coverage:

The PR adds no tests for the new caching functionality. Critical test cases needed:

1. Cache Hit/Miss Tests

   • Verify cache returns same result for identical files
   • Verify upload is skipped on cache hit
   • Verify upload occurs on cache miss

2. Cache Key Generation Tests

   • Test URL caching (same URL = same key)
   • Test file caching (same file = same key)
   • Test that modified files get different keys
   • Test that renamed files with same content get different keys

3. Configuration Tests

   • Verify cache_uploads=False prevents caching
   • Verify cache_uploads=True enables caching

4. Edge Cases

   • Non-existent files
   • Files modified during upload
   • Special characters in filenames/URLs
   • Very large files (performance)
   • Multiple AssetUploader instances (cache isolation)

5. Integration Tests

   • Test cache behavior across multiple upload calls
   • Test cache doesn't interfere with different file types

Recommendation: Add unit tests in a new file tests/unit/test_asset_uploader.py or similar

---

Additional Recommendations

1. Documentation Updates Needed

   • Update UploadConfig docstring to document cache_uploads parameter (upload_config.py:16)
   • Add docstring to upload_asset() method explaining caching behavior
   • Document cache scope (instance-level, not global)
   • Add usage examples in README or docs

2. Configuration Naming Convention

   • cache_uploads uses snake_case while other fields use camelCase (maxWorkers, maxRetries, chunkSize)
   • Recommendation: Use cacheUploads for consistency

3. Logging Enhancement

   • Cache hit logging only uses debug level
   • Consider adding metrics for cache hit rate
   • Log cache size periodically for monitoring

4. Future Enhancement Ideas

   • Add clear_cache() method for manual cache management
   • Add cache statistics (hits, misses, size)
   • Implement persistent caching across sessions
   • Add cache warming capability

---

Summary Assessment

Severity Breakdown:

• 🔴 High: Memory leak risk (unbounded cache growth)
• 🟡 Medium: Missing test coverage, file stat error handling
• 🟢 Low: Naming inconsistencies, documentation gaps

Overall: The feature is well-implemented for basic use cases but needs attention to cache lifecycle management and test coverage before merging. The unbounded cache growth is the primary concern that should be addressed.

Recommendation: Request changes to address memory management and add basic test coverage.

[Copilot]

Pull Request Overview

This PR implements asset upload caching to avoid redundant uploads of the same assets. The caching system tracks uploaded assets using a combination of file metadata (path, size, modification time) for local files and URLs for remote assets.

• Adds a caching mechanism with configurable toggle to prevent duplicate asset uploads
• Introduces cache key generation based on file metadata and URLs
• Updates configuration to allow users to enable/disable upload caching

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
src/rapidata/rapidata_client/datapoints/_asset_uploader.py	Implements upload cache with key generation logic and cache lookup/storage
src/rapidata/rapidata_client/config/upload_config.py	Adds cache_uploads boolean field to control caching behavior

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The debug log 'Asset added to cache' is always executed regardless of whether caching is enabled or the asset was actually added to cache. Move this log statement inside the if rapidata_config.upload.cache_uploads: block on line 50.

Suggested change

	logger.debug("Asset added to cache")
	logger.debug("Asset added to cache")

[Copilot]

The _get_cache_key method calls os.stat(asset) without checking if the file exists first. This will raise FileNotFoundError for non-existent files. However, the file existence check happens later in upload_asset at line 44. This causes cache key generation to fail before the proper error handling. Consider adding existence check in _get_cache_key or handling the exception appropriately.

Suggested change

	stat = os.stat(asset)
	try:
	stat = os.stat(asset)
	except FileNotFoundError:
	raise FileNotFoundError(f"File not found: {asset}")

[Copilot]

The new cache_uploads field is not documented in the class docstring. Add documentation for this field explaining that it controls whether uploaded assets are cached to avoid duplicate uploads.

[claude]

PR Review: Upload Caching Implementation

Thank you for implementing upload caching! This is a valuable performance optimization. I've reviewed the changes and have the following feedback:

✅ Positives

1. Good design choice: Using cachetools.LRUCache is appropriate for this use case
2. Smart cache key: Including file size and modification time (st_mtime_ns) ensures cache invalidation when files change
3. Configurable: The cacheUploads config option allows users to disable caching if needed
4. Reasonable defaults: 100K cache size and enabled by default are sensible

🐛 Potential Issues

1. Cache Inconsistency Across Instances (Critical)

Each AssetUploader instance creates its own cache. Since AssetUploader is instantiated in 6 different places:

• rapidata_order_manager.py:65
• _datapoint_uploader.py:17
• _validation_rapid_uploader.py:24
• demographic_manager.py:18
• _participant.py:24
• rapidata_benchmark.py:61

Impact: The same file uploaded in different contexts won't benefit from caching. You're potentially uploading the same asset multiple times.

Recommendation: Consider making the cache a class variable or using a module-level cache that's shared across all instances:

_shared_upload_cache: LRUCache = None

def __init__(self, openapi_service: OpenAPIService, max_cache_size: int = 100_000):
    self.openapi_service = openapi_service
    if AssetUploader._shared_upload_cache is None:
        AssetUploader._shared_upload_cache = LRUCache(maxsize=max_cache_size)
    self._upload_cache = AssetUploader._shared_upload_cache

2. Race Condition in Cache Check (Moderate)

Lines 38-41 and 52-54 have a TOCTOU (Time-of-Check-Time-of-Use) issue. If caching is disabled between the check and the cache write, you could cache when you shouldn't.

Recommendation: Check cacheUploads before attempting cache lookup:

if rapidata_config.upload.cacheUploads:
    asset_key = self._get_cache_key(asset)
    if asset_key in self._upload_cache:
        logger.debug("Asset found in cache")
        return self._upload_cache[asset_key]

3. FileNotFoundError Moved Too Early (Moderate)

The file existence check moved from line 47-48 to inside _get_cache_key (26-27). This means:

• URLs now go through an unnecessary else branch and check
• The error is raised during cache key generation instead of during actual upload

Recommendation: Keep the file check where it was, or restructure the logic:

def _get_cache_key(self, asset: str) -> str:
    """Generate cache key for an asset."""
    if re.match(r"^https?://", asset):
        return asset
    
    # For file paths
    if not os.path.exists(asset):
        raise FileNotFoundError(f"File not found: {asset}")
    stat = os.stat(asset)
    return f"{asset}:{stat.st_size}:{stat.st_mtime_ns}"

4. Logging Inconsistency (Minor)

Line 54: logger.debug("Asset added to cache") runs even when cacheUploads=False. The log message should be inside the if block.

🧪 Test Coverage

Missing tests for:

1. Cache hit scenario (upload same file twice)
2. Cache invalidation (file modified between uploads)
3. Cache disabled via config
4. URL assets vs file assets caching
5. Cache size limits (what happens at 100K+ items)
6. Concurrent uploads of the same asset

Recommendation: Add unit tests in tests/ directory covering these scenarios.

🔒 Security Considerations

1. Memory usage: 100K cache size could consume significant memory with large file paths
2. Cache poisoning: Consider if malicious file paths could cause issues (seems okay with current implementation)

📝 Documentation

The UploadConfig docstring (lines 5-11 in upload_config.py) needs updating to document the new cacheUploads parameter.

Performance Considerations

1. os.stat() on every cache lookup adds overhead. Consider caching the _get_cache_key result or checking the cache before computing the key
2. For URLs, cache keys could be simpler (just the URL)

Summary

This is a solid feature implementation, but the shared cache issue significantly reduces its effectiveness. I'd recommend addressing issue #1 before merging to maximize the performance benefits.

Priority fixes: Issues #1 and #2
Nice-to-have: Issues #3, #4, tests, and documentation updates

[claude]

PR Review: Upload Caching Feature

Thanks for implementing upload caching! This is a valuable performance optimization. Here's my detailed review:

Code Quality & Best Practices ✅

Strengths:

• Clean implementation of LRU cache using cachetools
• Good cache key generation strategy combining path, size, and modification time for local files
• Proper integration with existing logging infrastructure
• Configuration flag allows users to disable caching if needed

Suggestions for Improvement:

1. Cache Key Generation Logic (src/rapidata/rapidata_client/datapoints/_asset_uploader.py:21-31)

   • The _get_cache_key method calls os.path.exists() and os.stat(), which are also called in the main upload flow. Consider optimizing to avoid duplicate system calls.
   • For URL assets, consider normalizing URLs (e.g., trailing slashes, query parameter ordering) to improve cache hit rates.

2. Configuration Documentation (src/rapidata/rapidata_client/config/upload_config.py:16)

   • The new cacheUploads field lacks a docstring. Please add documentation explaining what it does and when users might want to disable it.

Potential Bugs & Issues ⚠️

1. Cache Invalidation for Modified Files (Critical)

   • Current implementation uses st_mtime_ns which is good, but file content could change without modifying mtime in some edge cases (e.g., system clock adjustments, manual mtime manipulation).
   • Consider adding a file hash to the cache key for more robust invalidation, or document this limitation.

2. Multiple Cache Instances (Medium Priority)

   • AssetUploader is instantiated in 6 different places across the codebase:
     • RapidataOrderManager
     • DatapointUploader
     • DemographicManager
     • ValidationRapidUploader
     • Participant
     • RapidataBenchmark
   • Each instance maintains its own separate cache, reducing effectiveness. Consider:
     • Using a shared singleton cache across all instances, OR
     • Injecting a shared AssetUploader instance, OR
     • Documenting this behavior clearly

3. Thread Safety (Medium Priority)

   • LRUCache from cachetools is not thread-safe by default
   • With maxWorkers: 10 for concurrent uploads, race conditions could occur
   • Consider using cachetools.cached decorator with a lock, or TTLCache with thread-safety measures

4. Cache Bypass Check Timing (Minor)

   • Line 52-54: The cache bypass check happens AFTER the upload completes
   • This means uploads always happen, but caching is conditional
   • This seems intentional but could be confusing - consider a comment explaining why

Performance Considerations 🚀

Positive:

• LRU eviction strategy is appropriate for this use case
• 100,000 max cache size is generous and should handle most workflows
• Avoids redundant uploads which can significantly improve performance

Concerns:

1. Memory Usage

   • Each cache entry stores a string (asset path/URL + metadata) and response filename
   • With 100k entries, memory footprint should be manageable but could be significant in long-running processes
   • Consider making max_cache_size configurable via UploadConfig

2. Cache Key Computation

   • os.stat() system call on every upload attempt adds overhead
   • For frequently accessed files, this is still much faster than uploading, but consider caching stat results briefly

Security Concerns 🔒

1. File Path Validation

   • Cache keys include raw file paths which could potentially be exploited if paths come from untrusted sources
   • Current implementation seems safe but ensure path traversal attacks aren't possible in upstream code

2. Cache Poisoning

   • If an attacker can control file paths and modify files with the same metadata, they could potentially poison the cache
   • Low risk in typical usage, but worth considering for security-sensitive applications

Test Coverage 🧪

Critical Gap:

• No tests found for the new caching functionality
• Recommended test cases:
  1. Test cache hit for same file uploaded twice
  2. Test cache miss after file modification
  3. Test cache behavior with URLs
  4. Test cache bypass when cacheUploads=False
  5. Test cache size limits and LRU eviction
  6. Test thread safety with concurrent uploads
  7. Test cache key generation for edge cases (symlinks, special characters, etc.)

Recommendations Summary

High Priority:

1. Add comprehensive tests for the caching functionality
2. Address thread-safety concerns for concurrent uploads
3. Document the cacheUploads configuration option

Medium Priority:
4. Consider consolidating cache instances across the codebase
5. Make max_cache_size configurable
6. Add content hashing for more reliable cache invalidation

Low Priority:
7. Optimize duplicate stat() calls
8. Add URL normalization for better cache hits

Overall, this is a solid feature implementation! The main concerns are around thread safety and test coverage. Would you like help implementing any of these suggestions?

🤖 Generated with Claude Code

[claude]

Pull Request Review: Upload Caching

Overview

This PR adds upload caching functionality to prevent redundant uploads of the same assets. The implementation uses an LRU cache with intelligent cache key generation based on file properties.

Code Quality & Best Practices

Positive Aspects

• Clean implementation: The caching logic is well-integrated into the existing flow
• Smart cache key generation: Uses file size and modification time for local files, which effectively detects file changes
• Configurable: The cacheUploads config option allows users to disable caching if needed
• Good logging: Debug and info logs help with troubleshooting

Areas for Improvement

1. Thread Safety Concerns (Critical)

File: src/rapidata/rapidata_client/datapoints/_asset_uploader.py:17

The class-level cache _shared_upload_cache is shared across all instances but LRUCache is not thread-safe. Given that this SDK uses maxWorkers: 10 for concurrent uploads, this could lead to:

• Race conditions during cache reads/writes
• Corrupted cache state
• Incorrect cache hits/misses

Recommendation: Use threading.Lock or consider functools.lru_cache with a wrapper, or use a thread-safe cache implementation.

import threading

class AssetUploader:
    _shared_upload_cache: LRUCache = LRUCache(maxsize=100_000)
    _cache_lock = threading.Lock()
    
    def upload_asset(self, asset: str) -> str:
        with tracer.start_as_current_span("AssetUploader.upload_asset"):
            logger.debug("Uploading asset: %s", asset)
            assert isinstance(asset, str), "Asset must be a string"
            
            asset_key = self._get_cache_key(asset)
            
            with self._cache_lock:
                if asset_key in self._shared_upload_cache:
                    logger.debug("Asset found in cache")
                    return self._shared_upload_cache[asset_key]
            
            # Upload logic...
            
            if rapidata_config.upload.cacheUploads:
                with self._cache_lock:
                    self._shared_upload_cache[asset_key] = response.file_name
                logger.debug("Asset added to cache")

2. Cache Key Generation Logic Issue

File: src/rapidata/rapidata_client/datapoints/_asset_uploader.py:22-32

The _get_cache_key method calls os.path.exists() and os.stat() inside the method, but upload_asset already does the existence check. This leads to:

• Double file system calls for cache misses
• Potential race condition if file is deleted between cache key generation and upload
• FileNotFoundError thrown during cache key generation instead of during upload

Recommendation: Move the file existence check to upload_asset and handle it consistently:

def _get_cache_key(self, asset: str) -> str:
    """Generate cache key for an asset."""
    if re.match(r"^https?://", asset):
        return asset
    
    # Assume file exists; caller should validate
    stat = os.stat(asset)  # Will raise FileNotFoundError if missing
    return f"{asset}:{stat.st_size}:{stat.st_mtime_ns}"

3. Error Handling Gap

File: src/rapidata/rapidata_client/datapoints/_asset_uploader.py:39-42

If _get_cache_key() raises an exception (e.g., FileNotFoundError, permission errors), the upload will fail without attempting the actual upload. For URLs, this is fine, but for files, you might want to catch and handle specific exceptions.

4. Cache Size Configuration

File: src/rapidata/rapidata_client/datapoints/_asset_uploader.py:17

The cache size is hardcoded to 100,000 entries. This could be:

• Too large for memory-constrained environments
• Too small for large-scale operations

Recommendation: Make this configurable via UploadConfig:

# In upload_config.py
cacheMaxSize: int = Field(default=100_000)

# In _asset_uploader.py
def __init__(self, openapi_service: OpenAPIService):
    self.openapi_service = openapi_service
    # Initialize instance cache from shared cache or create new
    if not hasattr(AssetUploader, '_cache_initialized'):
        AssetUploader._shared_upload_cache = LRUCache(
            maxsize=rapidata_config.upload.cacheMaxSize
        )
        AssetUploader._cache_initialized = True

5. Potential Cache Invalidation Issue

File: src/rapidata/rapidata_client/datapoints/_asset_uploader.py:32

For local files, the cache key includes st_mtime_ns which is good, but:

• If a user uploads the same file from different paths (e.g., via symlink), it will cache separately
• Absolute paths mean the same file in different locations won't share cache entries

This might be intended behavior, but consider using content hashing (e.g., MD5/SHA256 of file) for true deduplication, though this adds I/O overhead.

Potential Bugs

1. Race condition in cache check-then-act pattern: Between checking if asset_key in self._shared_upload_cache and returning the value, another thread could evict that entry from the LRU cache, causing a KeyError.

2. Missing cache check for config: The cache lookup happens before checking rapidata_config.upload.cacheUploads. If caching is disabled, we shouldn't return cached values. Move the config check earlier:

if rapidata_config.upload.cacheUploads:
    asset_key = self._get_cache_key(asset)
    if asset_key in self._shared_upload_cache:
        logger.debug("Asset found in cache")
        return self._shared_upload_cache[asset_key]

Performance Considerations

Positive

• Excellent performance improvement: Avoids redundant network uploads and file I/O
• LRU eviction policy: Reasonable choice for this use case

Concerns

1. File stat calls: For cache misses, we call os.stat() during key generation. Consider caching the stat result if used elsewhere.
2. Memory usage: 100K entries could use significant memory depending on string lengths. Monitor memory usage in production.

Security Concerns

1. No cache poisoning protection: A malicious actor with file system access could manipulate modification times to force cache hits/misses.
2. Path traversal: No validation of file paths in cache keys. Consider normalizing paths with os.path.abspath() or os.path.realpath().
3. Sensitive data in cache: If uploading sensitive assets, cached filenames persist in memory. Consider:
   • Cache expiration policy
   • Clearing cache on shutdown
   • Documenting this behavior

Test Coverage

Critical Issue: No tests found for the new caching functionality.

Required Tests:

1. Test cache hit for identical file (same path, content, mtime)
2. Test cache miss when file is modified (different mtime)
3. Test cache miss for different files with same name
4. Test URL caching (same URL should hit cache)
5. Test cache disabled via config
6. Test thread safety with concurrent uploads
7. Test cache eviction (when exceeding maxsize)
8. Test error handling (missing files, permission errors)
9. Test cache key generation for edge cases (special characters in paths, etc.)

Documentation

The new cacheUploads field in UploadConfig needs documentation:

• What does it do?
• Performance implications?
• Memory usage considerations?
• Thread safety guarantees?

Summary

Priority Issues:

1. 🔴 Critical: Add thread safety to cache operations
2. 🔴 Critical: Add comprehensive test coverage
3. 🟡 High: Fix cache lookup logic to respect config setting
4. 🟡 High: Handle race condition in cache check-then-act

Recommended Actions:

1. Add threading.Lock for cache operations
2. Move cacheUploads config check before cache lookup
3. Write comprehensive tests for caching behavior
4. Consider making cache size configurable
5. Add documentation for the new feature

The core idea is excellent and will significantly improve performance for repeated uploads. With the threading fixes and tests, this will be a solid addition to the SDK.