feat: run retention on the supervisor - #24
Conversation
Retention was configurable with sensible defaults and bounded pruners existed, but nothing invoked them: the supervisor pruned dead process records only. Every actor call writes a durable message row, including queries, so history grew without bound until an application scheduled its own job, which every adopter had to discover independently. The monitor now prunes expired messages and process history on retention_interval, defaulting to one hour, with zero disabling it.
Greptile SummaryThe PR schedules message and process-history retention in a dedicated supervisor thread with configurable timing and bounded retry backoff.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (3): Last reviewed commit: "fix: back off failed retention passes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @pruned_at = monotonic_now | ||
| MessagePruner.new.prune | ||
| ProcessPruner.new.prune |
There was a problem hiding this comment.
When either pruner raises a transient database error, @pruned_at has already been updated, so the monitor skips retention for the full interval—one hour by default—and a message-pruning failure also prevents process pruning during that period.
| @pruned_at = monotonic_now | |
| MessagePruner.new.prune | |
| ProcessPruner.new.prune | |
| MessagePruner.new.prune | |
| ProcessPruner.new.prune | |
| @pruned_at = monotonic_now |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/solid_objects/supervisor.rb
Line: 149-151
Comment:
**Failed passes suppress retries**
When either pruner raises a transient database error, `@pruned_at` has already been updated, so the monitor skips retention for the full interval—one hour by default—and a message-pruning failure also prevents process pruning during that period.
```suggestion
MessagePruner.new.prune
ProcessPruner.new.prune
@pruned_at = monotonic_now
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in cc2cbb9. @pruned_at is gone entirely. Retention now runs its own loop, and a raised pass falls through to wait_for_next_retention rather than recording a timestamp it never earned, so the next tick retries. The failure is instrumented as supervisor.retention_failed.
Covered by test/integration/scheduled_retention_test.rb "a failing retention pass retries rather than deferring for the interval", which stubs prune_expired_records to raise and waits for at least two failures.
| MessagePruner.new.prune | ||
| ProcessPruner.new.prune |
There was a problem hiding this comment.
If production has a large expired-record backlog or a database lock wait, these unbounded pruning loops run synchronously in the sole monitor thread, preventing dead-role replacement and dead-process cleanup until pruning finishes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/solid_objects/supervisor.rb
Line: 150-151
Comment:
**Retention blocks role supervision**
If production has a large expired-record backlog or a database lock wait, these unbounded pruning loops run synchronously in the sole monitor thread, preventing dead-role replacement and dead-process cleanup until pruning finishes.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in cc2cbb9. Retention moved to a dedicated thread started alongside the monitor, so an unbounded pruning pass no longer holds back replace_dead_roles.
Covered by "a slow retention pass does not block role replacement": prune_expired_records is stubbed to sleep 10 while a role crashes 0.1s in, and the test asserts the replacement happens within 3s. It fails with Timeout::Error against the previous single-threaded version.
Shutdown was the other half of this: sleeping the full interval made stop wait out the nap, so the pause is taken in supervisor_monitor_interval steps that notice @started flipping false.
Greptile flagged two defects in the scheduled retention pass. A failed pass suppressed retries: the timestamp was recorded before pruning ran, so a transient lock or connection error deferred retention for the full interval, up to an hour by default. Retention now retries on the next tick, and the failure is instrumented as supervisor.retention_failed. Retention also shared the monitor thread with role replacement. Pruning is unbounded work against a table that only grows, so a slow pass held back the supervision that keeps a crashed role alive. It now runs on a dedicated thread. The pause between passes is taken in monitor-interval steps so shutdown does not wait out an hour-long sleep.
|
@greptileai review |
| ) | ||
| end | ||
| wait_for_next_retention | ||
| end |
There was a problem hiding this comment.
When either pruner raises a transient database error, the rescue path unconditionally calls wait_for_next_retention, which uses the positive retention_interval; with the default configuration, expired records remain unpruned for another hour instead of retrying on the next supervisor tick.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/solid_objects/supervisor.rb
Line: 157
Comment:
**Failed passes still defer retries**
When either pruner raises a transient database error, the rescue path unconditionally calls `wait_for_next_retention`, which uses the positive `retention_interval`; with the default configuration, expired records remain unpruned for another hour instead of retrying on the next supervisor tick.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.The previous fix removed the timestamp that suppressed retries but left the failure path waiting the configured interval, so a transient lock or connection error still deferred retention for up to an hour. Greptile caught that the reply overstated what the change did. A failed pass now retries at monitor cadence and doubles the pause per consecutive failure, capped by the retention interval, so recovery is prompt without polling a database that stays down once a second forever. The retry test now configures a 600 second interval, which the previous behaviour would have waited out. Also merge the duplicate Unreleased changelog heading left by the rebase.
|
You are right and my earlier reply was wrong. Removing Fixed properly in 15f0fdb: The test was the other half of your point: at @greptileai review |
1 participant
The one milestone I would call blocking for a production-ready claim.
The gap
Retention policy is configurable with sensible defaults, and
MessagePruner,ProcessPruner, andInstancePrunerall exist and are tested. Nothing invoked them. The supervisor's monitor pruned dead process records and nothing else.Every actor call writes a durable message row, including queries and attribute reads. So the default experience was: works well for weeks, then the messages table is the largest object in the database and claim queries slow down. Every adopter had to independently discover they needed to schedule pruning.
The change
The supervisor monitor prunes expired messages and process history alongside the dead-process cleanup it already performed.
retention_intervaldefaults to 3600 seconds; zero disables itdead_process_cleanup_intervalTests
Six: expired messages are pruned without being asked, a zero interval disables it, a second pass waits for the interval rather than running every monitor tick, pruning is instrumented with its count, a failing pass leaves the monitor alive, and a negative interval is rejected.
Two things this cost me, both worth recording
My test double contained
def run = sleep(0.01) until @shutdown. Ruby parses that as(def run = ...) until @shutdown, so the class body looped forever redefining the method, and the file hung at load with no output. An endless method definition cannot take a modifier keyword.The interval test originally asserted nothing was pruned during the first 200ms, which failed because retention correctly runs once at startup. The test was wrong, not the behaviour, so I fixed the test and left the startup pass, which matches the existing cleanup pattern.
Roadmap
Milestone 2 removed and folded into the supervisor entry, with remaining milestones renumbered, per #15.