feat: 'data retention is over' message across sandbox tabs#474
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
1baf9d3 to
01f7e2b
Compare
67345d0 to
4ced102
Compare
4ced102 to
3e4dece
Compare
3e4dece to
18c408d
Compare
Render a single shared message on a sandbox's monitoring/events/logs tabs once that data has aged past its 7-day TTL, replacing empty views and the old client-side logs retention check. - models: add retentionExpired to the sandbox details model + mappers - common/data-retention-expired.tsx: shared message component - monitoring/events/logs views: render it when retentionExpired is set - details resolver: also flag paused sandboxes paused longer than 7d (killed sandboxes use the backend flag; running/resumed sandboxes clear it) - spec sync + regenerated dashboard-api contract types
18c408d to
2761f07
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2761f07df2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // killed case is already flagged by the backend (mappedDetails). | ||
| const retentionExpired = | ||
| mappedDetails.retentionExpired || | ||
| (mappedDetails.state === 'paused' && isPastRetention(pausedAt)) |
There was a problem hiding this comment.
Use pause time, not expiry time, for retention
When lifecycle events are missing, pausedAt falls back to fallbackPausedAt, which for paused infra sandboxes is mappedDetails.endAt; the infra OpenAPI spec documents endAt as the sandbox expiration time, not the pause time. In the exact case this feature is meant to handle—events for a sandbox paused longer than the retention window have aged out—this value is usually future/expiry-based, so isPastRetention(pausedAt) stays false and the logs/metrics/events tabs keep rendering unavailable data instead of the retention-expired message.
Useful? React with 👍 / 👎.
| import { LOG_RETENTION_MS } from '@/configs/logs' | ||
| import DashboardEmptyFrame from '@/features/dashboard/common/empty-frame' | ||
|
|
||
| const RETENTION_DAYS = LOG_RETENTION_MS / 24 / 60 / 60 / 1000 |
There was a problem hiding this comment.
🟡 The retention window shown in data-retention-expired.tsx is derived from LOG_RETENTION_MS (@/configs/logs), but the server-side paused-sandbox check in sandbox.ts:isPastRetention uses SANDBOX_MONITORING_METRICS_RETENTION_MS (@/features/dashboard/sandbox/monitoring/utils/constants). Both equal 7 days today, so nothing is broken, but the banner text and the gating threshold are now backed by two independent constants and could silently drift if either is updated. Consider importing SANDBOX_MONITORING_METRICS_RETENTION_MS into the banner so both come from a single source.
Extended reasoning...
What is happening
src/features/dashboard/sandbox/common/data-retention-expired.tsx:1-4 derives the displayed number of days from LOG_RETENTION_MS (@/configs/logs), which is currently 7 * 24 * 60 * 60 * 1000. Meanwhile, the server-side isPastRetention helper introduced in src/core/server/api/routers/sandbox.ts:23-32 uses SANDBOX_MONITORING_METRICS_RETENTION_MS (@/features/dashboard/sandbox/monitoring/utils/constants), which is 7 * millisecondsInDay. These are two independent constants that happen to be equal today.
Why the existing code doesn't prevent drift
Before this PR, LOG_RETENTION_MS had a legitimate log-scoped use in checkIfSandboxStillHasLogs. That caller has been removed, and the banner in data-retention-expired.tsx is now the sole consumer of LOG_RETENTION_MS for retention display purposes. But the banner text advertises the window that gates monitoring, events, and logs — which is actually enforced by SANDBOX_MONITORING_METRICS_RETENTION_MS (paused-sandbox path) and by the backend retentionExpired flag (killed path), neither of which is LOG_RETENTION_MS.
Step-by-step proof of the drift
- Today:
LOG_RETENTION_MS = 7 days,SANDBOX_MONITORING_METRICS_RETENTION_MS = 7 days. Banner says "7-day retention limit" and the check fires at 7 days. Consistent. - Say infra decides to extend monitoring retention to 14 days. Someone updates
SANDBOX_MONITORING_METRICS_RETENTION_MSto14 * millisecondsInDayand updates the infraretentionExpiredcutoff accordingly. Grep forSANDBOX_MONITORING_METRICS_RETENTION_MS— the banner file does not show up, so it is left atLOG_RETENTION_MS. - A user visits a paused sandbox that was paused 10 days ago.
isPastRetention(pausedAt)returnsfalse(10 < 14), soretentionExpiredisfalseand the actual data renders — but the user was never going to see the banner in that case anyway. - A user visits a paused sandbox that was paused 15 days ago. The check fires (retention expired), the banner renders, and it reads "exceeded the 7-day retention limit" — while the real threshold is 14 days.
Impact and severity
This is exactly the drift the finder describes. It is not a functional bug in this PR — both constants read 7 today, so the message is accurate. The refutation is correct that no incorrect behavior exists right now. However, this PR is the change that creates the inconsistency: it introduces the new server-side check pinned to SANDBOX_MONITORING_METRICS_RETENTION_MS while keeping the banner pinned to LOG_RETENTION_MS, and it removes the last legitimate log-scoped use of LOG_RETENTION_MS in this flow. So this is a maintainability nit worth flagging, not a merge blocker.
Fix
Replace LOG_RETENTION_MS in data-retention-expired.tsx with SANDBOX_MONITORING_METRICS_RETENTION_MS from @/features/dashboard/sandbox/monitoring/utils/constants. One line, keeps the client display and the server check tied to the same constant.
What
Shows a single, consistent "Data retention is over" message on a sandbox's monitoring / events / logs tabs once that data has aged past its 7-day TTL — replacing empty views and the old ad-hoc, client-side logs check.
Changes
models.ts: add `retentionExpired` to the sandbox details model; map it from the backend record (killed) and default `false` for live infra detail.common/data-retention-expired.tsx: new shared message component.sandbox.ts(detailsresolver): also flag paused sandboxes whose last pause is older than 7 days (the backend already exposes the pause time via `endAt`/lifecycle `pausedAt`). Resumed/running sandboxes clear the flag automatically; killed sandboxes use the backend flag.Matrix
Companion