feat(webapp): enforce watch plan limits - #4556
Conversation
Refuse a watch whose window exceeds the plan's agentWatchMaxHours, or that would push the org past its agentWatchers count, with a new watch_limit_reached result carrying an upgrade hint. Plan limits are a floor below the existing code ceilings (min(plan, WATCH_MAX_HOURS=24) and the per-chat cap of 3, which still apply independently). Fails open: an absent limit resolves to unlimited, so self-hosted is unaffected and the upgrade nudge is gated on billing presence. TRI-12863
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…63' into feat/agent-watch-limits-tri-12863
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // Plan floors sit below the code ceilings (min(plan, ceiling)). Fails open: an absent | ||
| // limit resolves to unlimited, so neither floor bites on self-hosted. | ||
| const planLimits = await resolveLimits(environment.organizationId); | ||
| if (spec.maxHours > effectiveWatchMaxHours(planLimits.maxHours)) { | ||
| return { | ||
| ok: false, | ||
| code: "watch_limit_reached", | ||
| error: hint("That watch window is longer than your plan allows."), | ||
| }; | ||
| } |
There was a problem hiding this comment.
🔍 Window floor also refuses one-shots, unlike the watcher-count floor
The window check runs before the immediate check, while the watcher-count check deliberately runs after it (apps/webapp/app/services/dashboardAgentWatches.server.ts:364-374) so that a one-shot consumes no slot. Consequence: a request whose condition is already satisfied — which would create no row at all — is still refused purely because the requested window exceeds the plan's agentWatchMaxHours. If the intent is "plan limits constrain what is actually persisted", the window check should arguably also sit after the immediate check, or the card should clamp the window instead of refusing.
Was this helpful? React with 👍 or 👎 to provide feedback.
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
The card-submit route's status ladder didn't handle watch_limit_reached, so a plan-limit refusal fell through to HTTP 500. Match the MCP route and return 409.
drizzle-kit generated them unformatted, failing the oxfmt --check code-quality gate.
| // Past the cap the card will never render; force it terminal without the render | ||
| // path so it leaves the queue instead of looping forever. | ||
| if (attempts !== null && attempts >= MAX_SWEEP_ATTEMPTS) { | ||
| try { | ||
| await forceAbandon({ id: investigation.id, note: UNSETTLED_INVESTIGATION_NOTE }); | ||
| result.abandoned++; | ||
| logger.warn( | ||
| "Dashboard agent investigation sweep: abandoned a card past the attempt cap", | ||
| { | ||
| investigationId: investigation.id, | ||
| chatId: investigation.chatId, | ||
| attempts, | ||
| } | ||
| ); | ||
| continue; | ||
| } catch (abandonError) { | ||
| logger.error("Dashboard agent investigation sweep: failed to abandon a poison card", { | ||
| investigationId: investigation.id, | ||
| chatId: investigation.chatId, | ||
| error: abandonError, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Investigations that fail for temporary reasons get permanently closed with no visible answer
Every failed clean-up attempt on an investigation is counted the same way (recordAttempt at apps/webapp/app/services/dashboardAgentInvestigationSweep.server.ts:122) regardless of whether the failure was temporary, so after five such failures the investigation is closed without ever showing its closing message to the user.
Impact: A user can be left with a permanently spinning investigation card that never resolves, even though the underlying investigation was perfectly displayable and only hit transient database or delivery errors.
Why transient failures accumulate toward the poison-row cap
sweepDashboardAgentInvestigations treats any throw from settleAndClose as evidence that the card "will never render" (comment at apps/webapp/app/services/dashboardAgentInvestigationSweep.server.ts:131-133). But settleInvestigationAndCloseCard (internal-packages/dashboard-agent-db/src/queries.ts:1140-1162) can throw for reasons other than an unrenderable state — any error in the transaction (append conflict, connection blip, statement timeout) surfaces the same way. The counter sweepAttempts is monotonic and is never reset on a successful run or after a long quiet period, so unrelated failures spread over the row's lifetime add up. Worse, a failed run rethrows at the end (:105-109) so the job retries immediately, and each retry increments the counter again — a short-lived fault affecting only the settle transaction can burn all five attempts within seconds. At the cap, forceAbandon (settleInvestigationAsInconclusive) marks the row terminal without appending the closing card, which is exactly the permanent spinner the transactional design was built to avoid.
A narrower trigger (e.g. only counting attempts when the error indicates an unrenderable state, or resetting/aging the counter) would keep the starvation fix without abandoning healthy rows.
Prompt for agents
In apps/webapp/app/services/dashboardAgentInvestigationSweep.server.ts, the attempt counter that drives force-abandonment is incremented for every failure of settleAndClose, not just for failures caused by an unrenderable investigation state. settleInvestigationAndCloseCard throws a specific error for the unrenderable case, but it can also throw for transient reasons (connection errors, timeouts, append conflicts). Because a failed run rethrows and the job is retried, a short transient fault can burn through MAX_SWEEP_ATTEMPTS quickly, after which the row is settled without its closing card — leaving the user's investigation card spinning forever, which is precisely what the transactional settle exists to prevent. Consider distinguishing the unrenderable-state error from other failures (e.g. a typed/marker error thrown by settleInvestigationAndCloseCard) and only counting attempts for that case, and/or ageing the counter so unrelated failures spread over time don't accumulate.
Was this helpful? React with 👍 or 👎 to provide feedback.
| try { | ||
| await forceAbandon({ id: investigation.id, note: UNSETTLED_INVESTIGATION_NOTE }); | ||
| result.abandoned++; | ||
| logger.warn( | ||
| "Dashboard agent investigation sweep: abandoned a card past the attempt cap", | ||
| { | ||
| investigationId: investigation.id, | ||
| chatId: investigation.chatId, | ||
| attempts, | ||
| } | ||
| ); | ||
| continue; |
There was a problem hiding this comment.
🟡 Clean-up run reports investigations as force-closed when another process already closed them
An investigation is counted and logged as force-closed (forceAbandon at apps/webapp/app/services/dashboardAgentInvestigationSweep.server.ts:135-145) even when the call reports that nothing was changed because it had already been closed normally, so the run's reported figures and warnings misstate what happened.
Impact: Operators see warnings about abandoned investigations that were actually settled cleanly, making it harder to spot genuinely stuck ones.
Mechanism
settleInvestigationAsInconclusive returns null when the row is no longer in_progress (internal-packages/dashboard-agent-db/src/queries.ts:1097-1125), i.e. a concluding turn or another sweep won the race. The sweep ignores the return value, unconditionally doing result.abandoned++ and emitting logger.warn("...abandoned a card past the attempt cap"). The correct classification in that case is alreadySettled.
| try { | |
| await forceAbandon({ id: investigation.id, note: UNSETTLED_INVESTIGATION_NOTE }); | |
| result.abandoned++; | |
| logger.warn( | |
| "Dashboard agent investigation sweep: abandoned a card past the attempt cap", | |
| { | |
| investigationId: investigation.id, | |
| chatId: investigation.chatId, | |
| attempts, | |
| } | |
| ); | |
| continue; | |
| try { | |
| const abandoned = await forceAbandon({ | |
| id: investigation.id, | |
| note: UNSETTLED_INVESTIGATION_NOTE, | |
| }); | |
| if (!abandoned) { | |
| result.alreadySettled++; | |
| continue; | |
| } | |
| result.abandoned++; | |
| logger.warn( | |
| "Dashboard agent investigation sweep: abandoned a card past the attempt cap", | |
| { | |
| investigationId: investigation.id, | |
| chatId: investigation.chatId, | |
| attempts, | |
| } | |
| ); | |
| continue; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| @@ -0,0 +1,53 @@ | |||
| import type { Limits } from "@trigger.dev/platform"; | |||
| import { WATCH_MAX_HOURS } from "@internal/dashboard-agent-contracts"; | |||
| import { getCachedLimit, isBillingConfigured } from "./platform.v3.server"; | |||
There was a problem hiding this comment.
🔍 Watch service now transitively constructs the platform cache (Redis client) at import time
dashboardAgentWatchLimits.server.ts imports platform.v3.server, whose module-level platformCache singleton constructs a RedisCacheStore (and hence an ioredis client) as a side effect of import (apps/webapp/app/services/platform.v3.server.ts:156-209). Because dashboardAgentWatches.server.ts now imports the limits module statically, every consumer of the watch service — including the several existing postgres-only test files that import it without a Redis container — will open that connection at import. Worth confirming the existing watch test suites still exit cleanly (no open handles / connection-retry noise); the existing pattern elsewhere (realtimeClientGlobal.server.ts) keeps the cached-limit provider behind a configuration module for this reason.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
What & why. Watches now honour a plan's watch limits. A watch whose window exceeds the plan's
agentWatchMaxHours, or that would push the org past itsagentWatcherscount, is refused with a newwatch_limit_reachedresult and an upgrade hint (a chat line on the card, HTTP 409 on the API).Key decisions.
min(plan, WATCH_MAX_HOURS=24)for the window, and the per-chat cap of 3 still applies independently. Plans only tighten, never loosen.isBillingConfigured().TRI-12863