feat: echo the active retention windows in the dashboard#499
Merged
Conversation
The windows live in the worker's SchedulerConfig, so a dashboard in another process cannot see them. The elected cleaner now records what it applies under retention:effective:<namespace> before each sweep.
Adds Queue.effective_retention() and GET /api/retention. The retention: prefix joins the reserved settings namespaces — the published policy is a report, not a knob.
Adds queue.effectiveRetention() and GET /api/retention. The retention: prefix joins the reserved settings namespaces — the published policy is a report, not a knob.
Adds Taskito.effectiveRetention() and GET /api/retention. The retention: prefix joins the reserved settings namespaces — the published policy is a report, not a knob.
Settings now explains why rows leave the listings: the per-table windows the retention leader reported, with distinct states for on, off, and unreported.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughWorkers publish namespaced effective-retention snapshots. Java, Node, and Python SDKs expose them, dashboard APIs return them, reserved settings keys are protected, and the settings page displays retention status and windows. ChangesCore publication and SDK access
Dashboard integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CleanupWorker
participant SharedStorage
participant QueueSDK
participant DashboardAPI
participant SettingsPage
CleanupWorker->>SharedStorage: publish effective retention JSON
QueueSDK->>SharedStorage: read namespaced retention JSON
QueueSDK-->>DashboardAPI: provide effective retention snapshot
DashboardAPI-->>SettingsPage: return retention contract
SettingsPage->>SettingsPage: render status and retention windows
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
pratyush618
requested review from
pratyush618
and removed request for
pratyush618
July 22, 2026 17:59
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/taskito-core/BINDING_CONTRACT.md (1)
238-241: 🔒 Security & Privacy | 🔵 TrivialReserved-prefix guarantee is enforced per-SDK, not centrally.
The contract states the
retention:prefix must never be listed/written/deleted through generic KV endpoints, but nothing in the coreStoragetrait (e.g.set_setting) enforces this — it's each SDK's dashboard settings handler that must independently reject the prefix (as seen in the JavaSettingsHandlers.PROTECTED_PREFIXEScheck). That's fine as an initial implementation, but it means a missed check in any one language's port silently reopens the spoofing hole this section is meant to close.Worth tracking as a follow-up: consider a shared/central guard (e.g. in
Storage::set_setting/delete_settingor a shared helper each binding calls) so the guarantee doesn't rely on N independent re-implementations staying in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-core/BINDING_CONTRACT.md` around lines 238 - 241, The reserved retention: prefix is currently protected only by individual SDK settings handlers, leaving other bindings able to spoof the published policy. Add a shared central guard or reusable helper used by generic KV list, write, and delete paths, including Storage::set_setting and delete_setting where applicable, so retention: is rejected consistently alongside auth: across all SDKs.crates/taskito-core/src/scheduler/retention.rs (1)
145-153: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider logging a swallowed parse failure.
.ok()here maps a genuine deserialization error toNonewith no trace. That's the correct external contract (unparseable → "unreported"), but during a schema change or mixed-version rollout this makes a real bug indistinguishable from "no leader has swept yet" — an operator would just see the dashboard report unreported with no signal of why.♻️ Suggested tweak (keeps the same `Option` contract, adds a diagnostic)
let Some(raw) = storage.get_setting(&retention_setting_key(namespace))? else { return Ok(None); }; - Ok(serde_json::from_str(&raw).ok()) + Ok(serde_json::from_str(&raw) + .inspect_err(|e| log::warn!("published retention document is unparseable: {e}")) + .ok())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-core/src/scheduler/retention.rs` around lines 145 - 153, Update read_effective_retention to preserve the existing unparseable-to-None contract while logging deserialization failures from serde_json::from_str. Include the namespace or retention setting context and the parse error in the diagnostic, without changing successful parsing or missing-setting behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dashboard/src/features/settings/derived.ts`:
- Around line 182-189: Update formatRetentionWindow so the positive-duration
fallback never formats as 0 seconds: clamp the rounded millisecond-to-second
value to at least one before passing it to plural. Add a test covering a
positive sub-second window such as 1–499ms and expect “1 second”.
In `@dashboard/src/routes/settings.tsx`:
- Around line 15-20: The /settings loader currently blocks on retention data and
couples retention failures to the Settings loading state. In
dashboard/src/routes/settings.tsx lines 15-20, keep awaiting settingsQuery() but
prefetch retentionQuery() best-effort without letting failure reject the loader;
in dashboard/src/features/settings/components/retention-section.tsx lines 70-89,
handle retention loading or error states independently so they do not reuse the
Settings loader/skeleton state.
---
Nitpick comments:
In `@crates/taskito-core/BINDING_CONTRACT.md`:
- Around line 238-241: The reserved retention: prefix is currently protected
only by individual SDK settings handlers, leaving other bindings able to spoof
the published policy. Add a shared central guard or reusable helper used by
generic KV list, write, and delete paths, including Storage::set_setting and
delete_setting where applicable, so retention: is rejected consistently
alongside auth: across all SDKs.
In `@crates/taskito-core/src/scheduler/retention.rs`:
- Around line 145-153: Update read_effective_retention to preserve the existing
unparseable-to-None contract while logging deserialization failures from
serde_json::from_str. Include the namespace or retention setting context and the
parse error in the diagnostic, without changing successful parsing or
missing-setting behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d4ee120-86f6-4471-8794-0dfa3e4b552b
📒 Files selected for processing (49)
crates/taskito-core/BINDING_CONTRACT.mdcrates/taskito-core/src/lib.rscrates/taskito-core/src/scheduler/maintenance.rscrates/taskito-core/src/scheduler/mod.rscrates/taskito-core/src/scheduler/retention.rscrates/taskito-java/src/queue/admin.rscrates/taskito-node/src/queue/admin.rscrates/taskito-python/src/py_queue/mod.rsdashboard/src/features/settings/api.tsdashboard/src/features/settings/components/retention-section.tsxdashboard/src/features/settings/derived.test.tsdashboard/src/features/settings/derived.tsdashboard/src/features/settings/hooks.tsdashboard/src/features/settings/index.tsdashboard/src/features/settings/types.tsdashboard/src/routes/settings.tsxdocs/content/docs/java/guides/operations/dashboard.mdxdocs/content/docs/node/guides/operations/dashboard-api.mdxdocs/content/docs/python/guides/dashboard/rest-api.mdxdocs/content/docs/python/guides/operations/job-management.mdxsdks/java/src/main/java/org/byteveda/taskito/DefaultTaskito.javasdks/java/src/main/java/org/byteveda/taskito/Taskito.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/DashboardServer.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/api/Contract.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/api/RetentionHandlers.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/api/SettingsHandlers.javasdks/java/src/main/java/org/byteveda/taskito/internal/JniQueueBackend.javasdks/java/src/main/java/org/byteveda/taskito/internal/NativeQueue.javasdks/java/src/main/java/org/byteveda/taskito/model/EffectiveRetention.javasdks/java/src/main/java/org/byteveda/taskito/spi/QueueBackend.javasdks/java/src/test/java/org/byteveda/taskito/dashboard/DashboardRetentionTest.javasdks/node/src/dashboard/handlers/index.tssdks/node/src/dashboard/handlers/retention.tssdks/node/src/dashboard/handlers/settings.tssdks/node/src/dashboard/routes.tssdks/node/src/index.tssdks/node/src/queue.tssdks/node/src/retention.tssdks/node/src/types.tssdks/node/test/dashboard/retention.test.tssdks/python/taskito/__init__.pysdks/python/taskito/_taskito.pyisdks/python/taskito/async_support/mixins.pysdks/python/taskito/dashboard/handlers/retention.pysdks/python/taskito/dashboard/handlers/settings.pysdks/python/taskito/dashboard/routes.pysdks/python/taskito/mixins/inspection.pysdks/python/taskito/retention.pysdks/python/tests/dashboard/test_dashboard_retention.py
The loader awaited GET /api/retention, so a backend that failed it replaced the whole page — branding, integrations, links — with an error page.
Rounding 1-499ms to "0 seconds" read as the immediate purge it isn't.
pratyush618
approved these changes
Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #489.
The dashboard never mentioned retention, so rows silently vanished from its
listings once a window elapsed — with retention on by default since #448, every
operator meets that blind.
Why the dashboard can't just read its own config
Retention is
SchedulerConfig.retention, resolved byeffective_retention()inside the worker process. Python carries it on the
Queueconstructor;the Node and Java shells carry it as a worker option the dashboard process
never sees. A dashboard computing the windows locally would print the
recommended defaults while a worker elsewhere ran with retention disabled.
So the cleaner publishes what it applies
auto_cleanup— the elected retention leader, whose config actually governs thedeletes — writes its resolved windows to
retention:effective:<namespace>before each sweep, and every dashboard echoes that document.
{ "enabled": true, "defaulted": true, "namespace": "default", "reported_at": 1753200000000, "windows": { "task_logs_ttl_ms": 259200000, "archived_jobs_ttl_ms": 604800000, "job_errors_ttl_ms": 604800000, "task_metrics_ttl_ms": 604800000, "dead_letter_ttl_ms": 2592000000 } }Three states stay distinct at every layer: unreported (no worker has swept —
the policy is unknown), enabled, and disabled.
retention:joins thereserved settings prefixes in all three shells: the document is a report, not a
knob, so it is neither listed nor writable through
/api/settings.Surface
GET /api/retentionin all three shellsQueue.effective_retention()/aeffective_retention()(Python),queue.effectiveRetention()(Node),Taskito.effectiveRetention()(Java)off / not-reported badge, a warning when the recommended defaults are in
force, and the reporting namespace and time
BINDING_CONTRACT.md; docs cover the endpoint and thePython accessor
The windows appear after the leader's first cleanup tick (
cleanup_interval,1200 ticks by default), so a fresh deployment shows the unreported state for a
minute or two. Publishing at startup was rejected: a non-leader's config does
not govern the deletes, and two differently-configured workers would flap the
document.
Verification
cargo test --workspace+ clippy +--features postgres/redischecks ·Python 1285 passed / 7 skipped (incl. a worker-driven publish end to end) ·
Node 469 vitest · Java
:testincl. 5 new dashboard cases · dashboardpnpm ci(biome, tsc, vitest, build) · docs build · both UI states checked ina real browser against a live dashboard.
Summary by CodeRabbit
GET /api/retentionto the dashboard and a new Settings UI section to display the latest worker-reported policy.