Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 1 addition & 10 deletions backend/ee/onyx/server/usage_limits.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
"""EE Usage limits - trial detection via billing information."""

from datetime import datetime
from datetime import timezone

from ee.onyx.server.tenants.billing import fetch_billing_information
from ee.onyx.server.tenants.models import BillingInformation
from ee.onyx.server.tenants.models import SubscriptionStatusResponse
Expand Down Expand Up @@ -31,13 +28,7 @@ def is_tenant_on_trial(tenant_id: str) -> bool:
return True

if isinstance(billing_info, BillingInformation):
# Check if trial is active
if billing_info.trial_end is not None:
now = datetime.now(timezone.utc)
# Trial active if trial_end is in the future
# and subscription status indicates trialing
if billing_info.trial_end > now and billing_info.status == "trialing":
return True
return billing_info.status == "trialing"
Comment on lines 30 to +31
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing the trial_end date check could lead to edge cases where a tenant is incorrectly classified as on trial. The previous implementation checked both trial_end > now AND status == "trialing" to ensure the trial period was actually still active.

Consider these scenarios:

  1. Stripe status update delays: If there's any delay in Stripe updating the subscription status after the trial ends, tenants could continue receiving trial limits
  2. Clock skew: The control plane and data plane might have slight time differences
  3. Grace periods: Some systems may want to continue trial status briefly after trial_end for grace periods

The previous implementation was more defensive and explicit. If this change is intentional (e.g., you've verified Stripe always updates status immediately), it should be documented in the PR description or as a code comment explaining why the date check is no longer needed.

Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/ee/onyx/server/usage_limits.py
Line: 30:31

Comment:
Removing the `trial_end` date check could lead to edge cases where a tenant is incorrectly classified as on trial. The previous implementation checked both `trial_end > now` AND `status == "trialing"` to ensure the trial period was actually still active.

Consider these scenarios:
1. **Stripe status update delays**: If there's any delay in Stripe updating the subscription status after the trial ends, tenants could continue receiving trial limits
2. **Clock skew**: The control plane and data plane might have slight time differences
3. **Grace periods**: Some systems may want to continue trial status briefly after trial_end for grace periods

The previous implementation was more defensive and explicit. If this change is intentional (e.g., you've verified Stripe always updates status immediately), it should be documented in the PR description or as a code comment explaining why the date check is no longer needed.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Contributor

Choose a reason for hiding this comment

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

is this valid?


return False

Expand Down
4 changes: 2 additions & 2 deletions backend/shared_configs/configs.py
Original file line number Diff line number Diff line change
Expand Up @@ -236,10 +236,10 @@ async def async_return_default_schema(*args: Any, **kwargs: Any) -> str:

# Per-week chunks indexed limits
USAGE_LIMIT_CHUNKS_INDEXED_TRIAL = int(
os.environ.get("USAGE_LIMIT_CHUNKS_INDEXED_TRIAL", "10000")
os.environ.get("USAGE_LIMIT_CHUNKS_INDEXED_TRIAL", 100_000)
Copy link
Contributor

Choose a reason for hiding this comment

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

The default value should be a string to maintain consistency with other config variables in this file. All other int(os.environ.get(...)) patterns use string defaults (e.g., lines 226, 231, 234, 246, 247, 251, 254).

Suggested change
os.environ.get("USAGE_LIMIT_CHUNKS_INDEXED_TRIAL", 100_000)
os.environ.get("USAGE_LIMIT_CHUNKS_INDEXED_TRIAL", "100000")

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/shared_configs/configs.py
Line: 239:239

Comment:
The default value should be a string to maintain consistency with other config variables in this file. All other `int(os.environ.get(...))` patterns use string defaults (e.g., lines 226, 231, 234, 246, 247, 251, 254).

```suggestion
    os.environ.get("USAGE_LIMIT_CHUNKS_INDEXED_TRIAL", "100000")
```

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.

)
USAGE_LIMIT_CHUNKS_INDEXED_PAID = int(
os.environ.get("USAGE_LIMIT_CHUNKS_INDEXED_PAID", "50000")
os.environ.get("USAGE_LIMIT_CHUNKS_INDEXED_PAID", 1_000_000)
Copy link
Contributor

Choose a reason for hiding this comment

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

The default value should be a string to maintain consistency with other config variables in this file. All other int(os.environ.get(...)) patterns use string defaults.

Suggested change
os.environ.get("USAGE_LIMIT_CHUNKS_INDEXED_PAID", 1_000_000)
os.environ.get("USAGE_LIMIT_CHUNKS_INDEXED_PAID", "1000000")

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/shared_configs/configs.py
Line: 242:242

Comment:
The default value should be a string to maintain consistency with other config variables in this file. All other `int(os.environ.get(...))` patterns use string defaults.

```suggestion
    os.environ.get("USAGE_LIMIT_CHUNKS_INDEXED_PAID", "1000000")
```

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.

)

# Per-week API calls using API keys or Personal Access Tokens
Expand Down
Loading