fix: address coderabbit review feedback

- Add try/except to _build_context, return None when hash fails
- Return None from _serialize_cache_key on total failure (no id()-based fallback)
- Replace hex-like test literal with non-secret placeholder

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Deep Mehta 2026-03-03 13:04:10 -08:00
parent da514866d6
commit 04097e6902
3 changed files with 23 additions and 14 deletions

View File

@ -242,11 +242,12 @@ def _canonicalize(obj: Any) -> Any:
return ("__repr__", repr(obj)) return ("__repr__", repr(obj))
def _serialize_cache_key(cache_key: Any) -> str: def _serialize_cache_key(cache_key: Any) -> Optional[str]:
""" """
Serialize cache key to a hex digest string for external storage. Serialize cache key to a hex digest string for external storage.
Returns SHA256 hex string suitable for Redis/database keys. Returns SHA256 hex string suitable for Redis/database keys,
or None if serialization fails entirely (fail-closed).
Note: Uses canonicalize + JSON serialization instead of pickle because Note: Uses canonicalize + JSON serialization instead of pickle because
pickle is NOT deterministic across Python sessions due to hash randomization pickle is NOT deterministic across Python sessions due to hash randomization
@ -263,8 +264,9 @@ def _serialize_cache_key(cache_key: Any) -> str:
try: try:
serialized = pickle.dumps(cache_key, protocol=4) serialized = pickle.dumps(cache_key, protocol=4)
return hashlib.sha256(serialized).hexdigest() return hashlib.sha256(serialized).hexdigest()
except Exception: except Exception as fallback_error:
return hashlib.sha256(str(id(cache_key)).encode()).hexdigest() _logger.warning(f"Failed pickle fallback for cache key: {fallback_error}")
return None
def _contains_nan(obj: Any) -> bool: def _contains_nan(obj: Any) -> bool:

View File

@ -316,14 +316,21 @@ class BasicCache:
return '' return ''
def _build_context(self, node_id, cache_key): def _build_context(self, node_id, cache_key):
"""Build CacheContext with hash. Returns None if hashing fails on NaN.""" """Build CacheContext with hash. Returns None if hashing fails."""
from comfy_execution.cache_provider import CacheContext, _serialize_cache_key from comfy_execution.cache_provider import CacheContext, _serialize_cache_key, _logger
return CacheContext( try:
prompt_id=self._current_prompt_id, cache_key_hash = _serialize_cache_key(cache_key)
node_id=node_id, if cache_key_hash is None:
class_type=self._get_class_type(node_id), return None
cache_key_hash=_serialize_cache_key(cache_key) return CacheContext(
) prompt_id=self._current_prompt_id,
node_id=node_id,
class_type=self._get_class_type(node_id),
cache_key_hash=cache_key_hash,
)
except Exception as e:
_logger.warning(f"Failed to build cache context for node {node_id}: {e}")
return None
async def _ensure_subcache(self, node_id, children_ids): async def _ensure_subcache(self, node_id, children_ids):
subcache_key = self.cache_key_set.get_subcache_key(node_id) subcache_key = self.cache_key_set.get_subcache_key(node_id)

View File

@ -333,13 +333,13 @@ class TestCacheContext:
prompt_id="prompt-123", prompt_id="prompt-123",
node_id="node-456", node_id="node-456",
class_type="KSampler", class_type="KSampler",
cache_key_hash="abcdef1234567890" * 4, cache_key_hash="a" * 64,
) )
assert context.prompt_id == "prompt-123" assert context.prompt_id == "prompt-123"
assert context.node_id == "node-456" assert context.node_id == "node-456"
assert context.class_type == "KSampler" assert context.class_type == "KSampler"
assert context.cache_key_hash == "abcdef1234567890" * 4 assert context.cache_key_hash == "a" * 64
class TestCacheValue: class TestCacheValue: