fix: revoke_reader owner guard (#215), until-only window anchoring (#214), take_sample slot race - #216
Draft
NikolayS wants to merge 6 commits into
Draft
fix: revoke_reader owner guard (#215), until-only window anchoring (#214), take_sample slot race#216NikolayS wants to merge 6 commits into
NikolayS wants to merge 6 commits into
Conversation
ash.revoke_reader() ends with REVOKE USAGE ON SCHEMA ash, so pointing it at the owner of schema ash removed the install's access to its own schema: take_sample(), status(), every reader and grant_reader() itself then failed with "permission denied for schema ash", with no supported way back. A superuser owner is immune through its ACL bypass, which is why the CI suite — which installs as postgres — never saw it. Refuse the three target classes that are not monitoring roles: the owner of schema ash, the calling role, and superusers. grant_reader() stays asymmetric on purpose; granting to any of them is harmless. Re-granting schema USAGE by hand still left grant_reader() failing with "permission denied for function _admin_funcs": ash._admin_funcs() was missing from the admin exclusion list, so the reader bundle covered it and revoking that bundle from the owner stripped the owner's own implicit EXECUTE on the helper grant_reader() initialises from. It is now admin-only; no reader calls it. Test: a dedicated database owned by a plain login role, with pg_ash installed as that role, asserting the refusal, that nothing was revoked, that grant_reader() still works, that a reader role never receives ash._admin_funcs(), and that revoking a real monitoring role is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014HzBGzjFyN8dXZHbdWmYBj
since and until were defaulted independently — since to now() - <span>,
until to now() — and since > until was never checked. So
`ash.top('wait_event', until => now() - interval '3 hours')` produced an
inverted window, the #63 degenerate-window guard rewrote it to
[now() - 1h, now() - 1h + 1min), and the reader returned a full,
plausible profile of a window three hours away with no NOTICE.
New ash._resolve_window() resolves both ends in one place, the way
ash.periods() already anchors: until defaults to now(), since defaults to
until - default_span so the span always hangs off the window END, and an
inverted window raises. aas, timeline, top, _samples, report (1 day span),
chart and summary all resolve through it; compare validates both of its
windows in its own frame so the error names ash.compare and not the
delegated ash.aas.
The #63 clamp is unchanged and still runs after resolution, on the
minute-floored int4 bounds, where it does its actual job of keeping
start_ts + 60 from wrapping past INT4_MAX.
Test: two seeded rollup blocks (target at until, decoy at now()) so the
until-only window is asserted by the values it returns, not just by its
bounds; an inverted window is asserted to raise on all eight readers;
and a zero-argument regression block asserts the exact default window and
that no reader rejects its own no-argument call — that block passes
identically on unpatched and patched SQL.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014HzBGzjFyN8dXZHbdWmYBj
v_current_slot is read once near the top of ash.take_sample() and drives both the query_map_N registration INSERT and the query_map_all join that packs map ids into sample.data — but the sample INSERT omitted the `slot` column, so it fell back to the table DEFAULT ash.current_slot(), which re-reads ash.config under the INSERT statement's own snapshot. ash.rotate() takes a different advisory objid than the sampler, so nothing serializes the two. A rotation committing between the dictionary read and the INSERT stored ids packed from the OLD slot's dictionary stamped with the NEW slot. The row still satisfies _sample_data_is_valid, so nothing warns and insert_errors is not bumped: the sample decodes to NULL query_ids, then to unrelated query_ids once the new slot's identity sequence re-issues 1, 2, 3... Name slot in the column list so the value read at the top is the value stored. Test: a real concurrent rotation cannot be landed deterministically in the middle of one take_sample() call from a single session, so the window is injected — ash.current_slot() is replaced, inside a transaction that is rolled back, with a probe returning the true slot on its first evaluation and a rotated slot on every later one, which is exactly the divergence a rotation opens. The stored slot must be the first value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014HzBGzjFyN8dXZHbdWmYBj
The #214 fix changed two user-visible things on eight readers: the window is now resolved from its END (until first, then since = until - span), and an inverted window raises. Three shipped contracts still stated the old rule, so a reader of the catalog, the blueprint, or the release notes was told something the code no longer does. ash.report's catalog comment ended "Returns null when the window has no coverage; never raises", which "select ash.report(now(), now() - interval '1 hour')" now falsifies. Of the two ways out — update the comment, or exempt report from the raise — this takes the comment, because: * the promise was always about DATA, not arguments. AAS_API.md §4 states it precisely: "Never raises for missing data ... if the whole window has no coverage at all, returns null". An inverted window is not missing data, it is a malformed request. * exempting report would have to answer it with null, which is the same value a legitimately uncovered window returns. A machine consumer that (correctly) treats null as "no coverage, skip ingestion" would then silently skip periods because of its OWN argument bug, with nothing in the payload to distinguish the two — reintroducing exactly the silent-wrong-answer class #214 removed, in the one reader built for automation. * report would become the only reader of the eight where a reversed pair is not an error, so a caller could not reason uniformly about the API. The narrow guarantee a consumer actually depends on ("null means no data, not a failure") is kept and spelled out. Also: aas/samples/report/chart/summary described their defaults as "last 1 hour"/"last 1 day" unconditionally, true only when until is null; AAS_API.md §1/§3/§4 and AAS_USER_STORIES.md §6 specified since -> now() - '1 hour'. All now state the anchored-on-until rule. AAS_USER_STORIES.md was not in the reported list but carries the identical claim and cross-references AAS_API.md §1, so it is corrected with them. RELEASE_NOTES.md had no entry for a change that introduces a new user-visible error class on eight readers; added one under the file's existing "## Fixes" section convention (there is no "## Fixes since 2.0 beta 1" heading on this branch to append to). No behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014HzBGzjFyN8dXZHbdWmYBj
The #215 fix made ash._admin_funcs() admin-only, but 2.0 beta 1 had already handed it to every configured reader. CREATE OR REPLACE preserves that ACL, while the corrected revoke_reader() now skips admin functions, so neither an upgrade nor the supported revoke/re-grant cycle could remove the stale capability. pg_monitor was affected on every beta1 install too. Extend the admin hardening pass to revoke explicit grants from every named non-owner grantee before re-granting the schema owner. The function owner is never revoked, and a supported secondary admin keeps access through membership in the owner role. Repeating the installer is therefore a fixed point instead of inheriting the prior version's reader bundle. Exercise the real beta1 tag in CI under a non-superuser owner, with both a named reader and pg_monitor carrying the old grant. Assert the stale grants disappear, the owner and inherited admin remain effective, a second re-apply produces the same ACL snapshot, and the upgraded snapshot matches a fresh install.
Add release-note entries for the revoke_reader owner guard, take_sample slot consistency, and admin-function ACL convergence. Call out the new errors and the installer re-apply consequence for direct admin grants.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #214 and #215, plus the
take_sample()slot race and an ACL-convergence gap the first twouncovered. All four are independent of the
rollup_1hgrain cluster (#210 / #131 / #150), so thiscan land without waiting on that decision.
Found by the pre-tag audit — see #160.
Draft on purpose. Per
CLAUDE.mdthis needs REV and your explicit approval. It has neither.What's here
01c54cerevoke_reader()bricked a non-superuser-owned install. It now refuses when the target owns schemaash, iscurrent_user, or is a superuser.ash._admin_funcs()moved into the admin set sorevoke_readerstops stripping the helpergrant_reader()initialises from.f1d9d7euntilsilently answered a window an arbitrary distance away.untilnow resolves first,sincedefaults tountilminus the reader's span, andsince > untilraises instead of being rewritten by the #63 clamp.724a0fatake_sample()resolved query-map ids against the slot it read at the top but omittedslotfrom the INSERT, so the column DEFAULT re-evaluated under a fresh snapshot. A rotation landing mid-sample stamped old-dictionary ids with the new slot: NULL attribution, then unrelated query ids once the new slot reused map ids 1,2,3. Passed validation, never touchedinsert_errors— undetectable after the fact.037896dblueprints/AAS_API.mdwith the #214 rule.04233f9_admin_funcs()became admin-only, an EXECUTE grant handed out by 2.0 beta 1 could never be withdrawn (CREATE OR REPLACEpreserves ACLs; the correctedrevoke_readerskips admin functions). A beta1-upgraded install kept privileges a fresh install does not grant,pg_monitorincluded. Hardening now revokes from named non-owner grantees, not just PUBLIC.1b1fac0Verification
Every fix was proven RED against unpatched SQL first, then GREEN — and independently
re-derived by a second reviewer working from a clean checkout of the same base commit, not from
the author's evidence.
until-only cases and nine inverted-window cases that now raise. Everything else byte-identical —
all ten zero-arg readers,
periods()still anchoring onuntil, the bug: _at readers raise 'integer out of range' on absurd timestamps — same class as #51 #63 clamp still applying.(288 catalog rows) all clean.
04233f9additionally asserts a second re-apply is a fixed point, which is the propertythat actually matters for convergence.
The zero-argument regression guard was explicitly checked for tautology: run alone against
unpatched SQL it passes, while the inverted-window block still fails. That is the PR #131
failure mode being guarded rather than reproduced.
Review notes
ash.report's "never raises" contract was reworded rather than exempted:"the only raise is the malformed request
since > until, so a null always means no data andnever a failed call." That keeps the guarantee machine consumers depend on (null ⇒ no data)
while being honest about argument validation. Worth a second opinion.
04233f9means an admin-function grant an operatorgave directly to a role is now removed on every installer apply. The supported path becomes
membership in the schema-owner role. Called out explicitly in the release notes.
🤖 Generated with Claude Code
https://claude.ai/code/session_014HzBGzjFyN8dXZHbdWmYBj