Skip to content

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

Closed

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

  1. IAmJSD commented on Oct 20, 2026

    @IAmJSD
    Collaborator

    Claiming: working on this

  2. self-assigned this
    on Oct 20, 2026
  3. added a commit that references this issue on Oct 25, 2026
    18a9d4d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions