Skip to content

chore(config): retention.execution_history_days is never read at runtime #88

Description

@cristim

retention.execution_history_days is a settings key that nothing reads.

Surfaced while reviewing PR LeanerCloud/cloud-commitments-cli#1521 (per-plan health score), where a lookback window was initially justified by citing this key as the project's execution-retention policy. It is not: the key is declared and never consumed.

Evidence

grep -rn "retention.execution_history_days" --include='*.go' . returns only:

  • internal/config/defaults.go:289 — the seed row (Value: 90)
  • internal/config/defaults_test.go:221 — a table assertion that the seed row exists
  • internal/config/README.md:215 — documentation describing it as the retention policy

No runtime reader. The value that actually governs execution retention is config.DefaultExecutionTTLDays (internal/config/constants.go:15, currently 30), consumed by internal/server/handler.go when it calls CleanupOldExecutions.

Why it matters

The key is seeded into the settings table and documented in the config README as controlling how long execution history is kept, so an operator can change it in the settings UI and reasonably believe they have extended or shortened retention. Nothing happens. Meanwhile the real horizon is a compile-time constant they cannot reach.

That gap is also an active trap for contributors: it reads as an authoritative source of truth, and PR LeanerCloud/cloud-commitments-cli#1521 cited it as exactly that before the discrepancy was caught in review.

The two values also disagree (90 in the dead key, 30 in the live constant), so there is no reading of the current state that is self-consistent.

Options

  1. Wire it up — have the cleanup path read the setting, falling back to DefaultExecutionTTLDays only when unset, and reconcile the two values. Note that anything deriving a window from retention (the plan health score's planHealthLookbackDays, internal/api/plan_health.go) then becomes runtime-configurable too and must read it rather than the constant.
  2. Remove it — drop the seed row, its test assertion, and the README section, leaving DefaultExecutionTTLDays as the single documented horizon. A migration would be needed to delete the row from existing installs.

Option 2 is the smaller change and matches how the value is actually treated today. Option 1 is the better product behaviour if per-install retention tuning is wanted.

Either way the README should end up describing whatever is actually true.

Not urgent

Nothing is broken at runtime: retention works, it is just not configurable the way the settings key implies. This is correctness-of-configuration debt, not a live defect.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions