Skip to content

feat(daemon): prune dead sessions past retention limits on startup#296

Open
mgabor3141 wants to merge 2 commits into
mainfrom
review/T22-dead-session-retention
Open

feat(daemon): prune dead sessions past retention limits on startup#296
mgabor3141 wants to merge 2 commits into
mainfrom
review/T22-dead-session-retention

Conversation

@mgabor3141

Copy link
Copy Markdown
Contributor

Implements review ticket T22.

Dead-but-not-dismissed sessions previously persisted forever — each keeps a meta.json plus up to ~2 MiB of rotated scrollback under ~/.local/state/gmux/sessions/<id>/, removed only on explicit dismiss or orphan-sweep. For heavy users who never dismiss, the state dir grew unbounded. This adds a retention policy applied during the startup Sweep: dead-session corpses older than MaxAge are aged out, then only the newest MaxCount survive (LRU by effective timestamp — ExitedAt, falling back to LastActivityAt then CreatedAt). Defaults are 30 days / 200 sessions, overridable via GMUX_SESSION_RETENTION_DAYS and GMUX_SESSION_RETENTION_MAX (0 disables a limit).

Safety rules: Sweep only loads Alive=false records, and any still-running runner re-registers as alive immediately afterwards, so live sessions are never on the chopping block. Records with no parseable timestamp are treated conservatively — never aged out and ranked as newest for the count cap — so they are never evicted unexpectedly. The clock is injectable (WithClock) for deterministic tests; the policy is injected via WithRetention, and New without options preserves the prior no-prune behavior.

Verification

  • cd services/gmuxd && go build ./... — passes
  • go test ./... — all packages pass, including new sessionmeta retention tests (age-out, count cap, no-policy, undatable-survives, env override)

Source finding: L1

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@github-actions

github-actions Bot commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Try this PR

curl -sSfL https://gmux.app/install-pr.sh | sh -s -- 296

Built from 7a18b00 — fix(daemon): reject overflowing GMUX_SESSION_RETENTION_DAYS
Requires GitHub CLI with auth. Artifacts expire after 7 days.

@mgabor3141 mgabor3141 force-pushed the review/T22-dead-session-retention branch from ab3384a to efc7f28 Compare June 7, 2026 10:36

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@mgabor3141

Copy link
Copy Markdown
Contributor Author

@greptile review

@greptile-apps

greptile-apps Bot commented Jun 11, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

Safe to merge — the retention logic is well-scoped to dead sessions only, the overflow guard is correct, and live sessions are protected by the existing Sweep contract.

The prune path is exercised by five focused tests with an injected clock, the env-override overflow guard is correct and regression-tested, and the wiring in main.go is a minimal one-liner. No changes touch live-session handling or existing persistence contracts.

No files require special attention.

Important Files Changed

Filename Overview
services/gmuxd/internal/sessionmeta/sessionmeta.go Core retention logic — adds RetentionPolicy, DefaultRetention (with env overrides + overflow guard), prune (age-out then count cap), and effectiveTime. Logic is sound; minor dead-code nit in prune's clock setup.
services/gmuxd/internal/sessionmeta/sessionmeta_test.go Five new retention tests covering age-out, count cap, no-policy, undatable-survives, and env overrides. Missing a case for GMUX_SESSION_RETENTION_MAX=0 disabling the count limit, but all happy paths are well covered.
services/gmuxd/cmd/gmuxd/main.go One-line wiring change: passes DefaultRetention() to New via WithRetention; clean and minimal.
packages/paths/paths.go Doc comment only — adds retention policy description to SessionDir; accurate and matches the implementation.

Sequence Diagram

sequenceDiagram
    participant main as gmuxd/main
    participant store as Store.Sweep
    participant prune as Store.prune
    participant disk as Disk (sessions/)

    main->>store: Sweep()
    store->>disk: ReadDir(sessions/)
    disk-->>store: []DirEntry
    loop each entry
        store->>disk: Read meta.json
        disk-->>store: "Session (Alive=false)"
    end
    store->>prune: prune(loaded)
    alt "MaxAge > 0"
        prune->>prune: "cutoff = now() - MaxAge"
        loop each session
            alt "effectiveTime < cutoff"
                prune->>disk: Remove(sessDir) [age eviction]
            else
                prune->>prune: add to survivors
            end
        end
    end
    alt "MaxCount > 0 and len(survivors) > MaxCount"
        prune->>prune: sort survivors newest-first
        loop sess in survivors[MaxCount:]
            prune->>disk: Remove(sessDir) [count eviction]
        end
        prune->>prune: truncate to MaxCount
    end
    prune-->>store: survivors
    store-->>main: []Session (pruned)
Loading

Reviews (3): Last reviewed commit: "fix(daemon): reject overflowing GMUX_SES..." | Re-trigger Greptile

Comment thread services/gmuxd/internal/sessionmeta/sessionmeta.go
mgabor3141 added a commit that referenced this pull request Jun 15, 2026
A day count above ~106 751 overflows time.Duration (int64 ns) when
multiplied by 24h, producing a negative MaxAge that prune silently
treats as 'no age limit'. Cap at the largest safe value so an
out-of-range setting is logged and ignored rather than quietly
disabling retention.

Addresses greptile review comment on PR #296.
A day count above ~106 751 overflows time.Duration (int64 ns) when
multiplied by 24h, producing a negative MaxAge that prune silently
treats as 'no age limit'. Cap at the largest safe value so an
out-of-range setting is logged and ignored rather than quietly
disabling retention.

Addresses greptile review comment on PR #296.
@mgabor3141 mgabor3141 force-pushed the review/T22-dead-session-retention branch from 4ec1678 to 7a18b00 Compare June 15, 2026 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant