Sweep the stale 21-day mentions #1941 left behind (#1937 follow-through) - #1942
Conversation
The review on #1941 listed them: thirteen comment sites and one operator-facing CLI line (the --backfill-rollups completion guidance) still described the 21-day world. The CLI line now interpolates HourlyRetentionInterval so it cannot drift again; current-behavior comments are corrected or horizon-neutralized; historical narration keeps its 21s because it describes the defect, not the behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// <para>Route thresholds sit a margin BELOW each retention horizon (raw kept 4d → route ≤3d; hourly kept 90d → | ||
| /// route ≤20d), so a drop lagging the boundary (1-day chunk granularity + the 3-day CAGG refresh) can never leave |
There was a problem hiding this comment.
This sweep changed "hourly kept 21d" to "hourly kept 90d" here but left the derived threshold as "route ≤20d" — that's now internally inconsistent. The doc explicitly says thresholds sit a 1-day margin below the horizon (RouteMargin = TimeSpan.FromDays(1), HourlyMaxAge = HourlyRetentionSpan - RouteMargin), so kept 90d should route ≤89d, not ≤20d. The old "21d → ≤20d" pairing was internally consistent (margin=1), but the mechanical 21→90 substitution wasn't applied to the derived "≤20d" figure. Compare with the very next member doc a few lines down (line 147), which correctly says "a day inside the 90-day hourly retention" (i.e. 89 days) — so this paragraph now contradicts the member doc directly beneath it, and reintroduces exactly the kind of stale/misleading number this PR exists to sweep away.
| /// <para>Route thresholds sit a margin BELOW each retention horizon (raw kept 4d → route ≤3d; hourly kept 90d → | |
| /// route ≤20d), so a drop lagging the boundary (1-day chunk granularity + the 3-day CAGG refresh) can never leave | |
| /// <para>Route thresholds sit a margin BELOW each retention horizon (raw kept 4d → route ≤3d; hourly kept 90d → | |
| /// route ≤89d), so a drop lagging the boundary (1-day chunk granularity + the 3-day CAGG refresh) can never leave |
| output.WriteLine("is the confirmation; the first purge then reclaims the raw tables in one pass."); | ||
| output.WriteLine(); | ||
| output.WriteLine("Do not delay the restart. The hourly rollups carry their OWN 21-day retention policy, already"); | ||
| output.WriteLine($"Do not delay the restart. The hourly rollups carry their OWN {TimescaleSupport.HourlyRetentionInterval} retention policy, already"); |
There was a problem hiding this comment.
Minor wording nit on the interpolated line, since the whole point of this change is that this exact string can't silently drift again: HourlyRetentionInterval is the noun phrase "90 days", but it's spliced into an adjectival slot ("carry their OWN 90 days retention policy"), which reads ungrammatically. The old literal text was correctly hyphenated as an adjective ("OWN 21-day retention policy"). Might be worth rephrasing to something like "...carry their OWN retention policy ({TimescaleSupport.HourlyRetentionInterval}), already..." so the interpolated value sits in a position where "90 days" reads naturally as a noun phrase rather than a compound adjective.
ReviewScope: comment/doc-string sweep plus one operator-facing CLI string, following up on #1937 (21-day → 90-day hourly retention horizon). Touches OverviewCleans up thirteen stale "21-day" mentions left after the retention horizon moved to 90 days, and makes the one operator-facing line ( Parity checkNo Issues found (posted inline)
Other notes
|
…d the adjectival splice Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review: comment/message sweep (#1937 follow-through)Verified this against the diff and the surrounding code. Overall this is exactly what the description claims — a low-risk documentation/message sweep with one behavior-relevant change (the CLI now reads the constant instead of a literal). What I checked:
No correctness, security, or performance concerns — this is comments/docs plus one string interpolation, no logic changes. Nothing further to flag. |
…stops overclaiming erikdarlingdata#1957: the SECURITY WARNING was honest. After applying the hardened ACL the installer set the service account as owner by handing Set-Acl a freshly constructed FileSecurity carrying nothing but that owner. Set-Acl applies the whole descriptor it is given, so that call also wrote an empty, unprotected DACL - re-enabling inheritance and handing BUILTIN\Users read back on an install under C:\, one statement after the hardening. Confirmed on a scratch layout under C:\ before changing anything: immediately after the owner step the file read protected=False with BUILTIN\Users present, all four inherited ACEs back and every hardened ACE gone. erikdarlingdata#1818's startup sweep re-hardened everything seconds later at the first service start, which is why the operator's own before/after captures both looked clean and only the installer disagreed. The owner now goes onto the file's CURRENT descriptor, so the hardened DACL rides along and the verified state is the final state. Proven by running the SHIPPING hardening block against the scratch layout: fresh install and a second install over the already-hardened layout both report 3 hardened / 0 warned with protected=True, no BUILTIN\Users, owner = the service SID; and a control where the harden genuinely fails still names the files, which are world-readable on disk at that moment. The per-file verification predicate is untouched - it was correct throughout. The install summary now states that the service re-verifies these ACLs at every start. erikdarlingdata#1958: the retention summary read "(raw 4 days, hourly CAGGs 90 days; daily CAGGs kept indefinitely)" - a universal claim with three counterexamples in timescaledb_information.jobs, where the docs send an operator to check it. query_store_stats_interval_hourly carries 7 days by design (erikdarlingdata#1849's dedup tier, sized to outlive raw), its daily twin carries 10, and the nine baseline aggregates keep 35. The line now names every tier that has a policy, with every number interpolated from its constant per erikdarlingdata#1942. Darling/README.md gains the rollup-tier ladder, which was not documented anywhere user-facing - its Retention section described only the collector horizons, so the three rolled-up raw tables read as 30 days when a 4-day policy is what actually drops them. Both fixes are newly pinned; neither line had a test before. The summary pin derives its expectation from RetentionPolicies itself, so a tier added on a new horizon fails until the summary mentions it. Watched red: the summary pin fails naming IntervalRetentionInterval against the old wording, and the installer pin fails both against the bare descriptor and against a second bare descriptor left after a correct re-read. Also fixes two pre-existing xUnit2013 warnings on dev in ProcedureStatsCollectorDefinitionTests, restoring the zero-warning build. Closes erikdarlingdata#1957 Closes erikdarlingdata#1958 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
The #1941 review bot listed pre-existing comment/message drift outside that PR's diff: thirteen comment sites and one operator-facing CLI line (the --backfill-rollups completion output still taught the 21-day trim timing - actively misleading after the horizon moved). This PR is that sweep:
Comment/message-only plus one interpolated string; service project builds 0 warnings.
🤖 Generated with Claude Code