mirror of
https://github.com/Significant-Gravitas/AutoGPT.git
synced 2026-04-08 03:00:28 -04:00
fix(backend): address second round CodeRabbit review feedback
- Narrow except blocks to (RedisError, ConnectionError, OSError) instead of bare Exception to avoid hiding coding bugs - Remove raw user_id from log messages to prevent PII leaks under Redis outages - Reuse credit_model instead of fetching twice in execute_block() - Treat post-execution InsufficientBalanceError as fatal (matches executor behavior) instead of silently swallowing it
This commit is contained in:
@@ -9,6 +9,7 @@ import logging
|
||||
from datetime import UTC, datetime, timedelta
|
||||
|
||||
from pydantic import BaseModel, Field
|
||||
from redis.exceptions import RedisError
|
||||
|
||||
from backend.data.redis_client import get_redis_async
|
||||
|
||||
@@ -89,7 +90,7 @@ async def _session_reset_from_ttl(
|
||||
ttl: int = await redis.ttl(_session_key(user_id, session_id)) # type: ignore[union-attr]
|
||||
if ttl > 0:
|
||||
return datetime.now(UTC) + timedelta(seconds=ttl)
|
||||
except Exception:
|
||||
except (RedisError, ConnectionError, OSError):
|
||||
pass
|
||||
# Key doesn't exist or has no TTL — use the configured TTL
|
||||
return datetime.now(UTC) + timedelta(seconds=_SESSION_TTL_SECONDS)
|
||||
@@ -118,7 +119,7 @@ async def get_usage_status(
|
||||
session_used = int(await redis.get(_session_key(user_id, session_id)) or 0)
|
||||
weekly_used = int(await redis.get(_weekly_key(user_id)) or 0)
|
||||
session_resets_at = await _session_reset_from_ttl(redis, user_id, session_id)
|
||||
except Exception:
|
||||
except (RedisError, ConnectionError, OSError):
|
||||
logger.warning("Redis unavailable for usage status, returning zeros")
|
||||
session_used = 0
|
||||
weekly_used = 0
|
||||
@@ -157,7 +158,7 @@ async def check_rate_limit(
|
||||
redis = await get_redis_async()
|
||||
session_used = int(await redis.get(_session_key(user_id, session_id)) or 0)
|
||||
weekly_used = int(await redis.get(_weekly_key(user_id)) or 0)
|
||||
except Exception:
|
||||
except (RedisError, ConnectionError, OSError):
|
||||
logger.warning("Redis unavailable for rate limit check, allowing request")
|
||||
return
|
||||
|
||||
@@ -205,9 +206,8 @@ async def record_token_usage(
|
||||
pipe.expire(w_key, max(seconds_until_reset, 1))
|
||||
|
||||
await pipe.execute()
|
||||
except Exception:
|
||||
except (RedisError, ConnectionError, OSError):
|
||||
logger.warning(
|
||||
"Redis unavailable for recording token usage (user=%s, tokens=%d)",
|
||||
user_id,
|
||||
"Redis unavailable for recording token usage (tokens=%d)",
|
||||
total,
|
||||
)
|
||||
|
||||
@@ -115,6 +115,7 @@ async def execute_block(
|
||||
|
||||
# Pre-execution credit check
|
||||
cost, cost_filter = block_usage_cost(block, input_data)
|
||||
credit_model = None
|
||||
if cost > 0:
|
||||
credit_model = await get_user_credit_model(user_id)
|
||||
balance = await credit_model.get_credits(user_id)
|
||||
@@ -136,9 +137,8 @@ async def execute_block(
|
||||
outputs[output_name].append(output_data)
|
||||
|
||||
# Charge credits for block execution
|
||||
if cost > 0:
|
||||
if cost > 0 and credit_model:
|
||||
try:
|
||||
credit_model = await get_user_credit_model(user_id)
|
||||
await credit_model.spend_credits(
|
||||
user_id=user_id,
|
||||
cost=cost,
|
||||
@@ -154,11 +154,14 @@ async def execute_block(
|
||||
),
|
||||
)
|
||||
except InsufficientBalanceError:
|
||||
# Block already executed — log warning but still return output
|
||||
logger.warning(
|
||||
"Insufficient credits to charge for block %s (user=%s)",
|
||||
block.name,
|
||||
user_id,
|
||||
# Concurrent spend drained balance after our pre-check passed.
|
||||
# Treat as fatal to avoid unpaid execution (matches executor behavior).
|
||||
return ErrorResponse(
|
||||
message=(
|
||||
f"Insufficient credits to charge for '{block.name}'. "
|
||||
"Please top up your credits to continue."
|
||||
),
|
||||
session_id=session_id,
|
||||
)
|
||||
|
||||
return BlockOutputResponse(
|
||||
|
||||
@@ -139,8 +139,8 @@ class TestExecuteBlockCreditCharging:
|
||||
# get_user_credit_model should not be called at all for zero-cost blocks
|
||||
mock_get_credit.assert_not_awaited()
|
||||
|
||||
async def test_still_returns_output_on_post_exec_insufficient_balance(self):
|
||||
"""If charging fails after execution, block output is still returned."""
|
||||
async def test_returns_error_on_post_exec_insufficient_balance(self):
|
||||
"""If charging fails after execution (concurrent spend race), return error."""
|
||||
from backend.util.exceptions import InsufficientBalanceError
|
||||
|
||||
block = _make_block()
|
||||
@@ -171,6 +171,6 @@ class TestExecuteBlockCreditCharging:
|
||||
matched_credentials={},
|
||||
)
|
||||
|
||||
# Output should still be returned even though charging failed
|
||||
assert isinstance(result, BlockOutputResponse)
|
||||
assert result.success is True
|
||||
# Post-exec charge failure is treated as fatal (matches executor behavior)
|
||||
assert isinstance(result, ErrorResponse)
|
||||
assert "Insufficient credits" in result.message
|
||||
|
||||
Reference in New Issue
Block a user