Roll an hour up without letting it lose one of its three states - #28
Merged
Conversation
presence_sample has been partitioned monthly since 0003 for rollups and retention that nobody had written. This writes them. The rollup keeps the tally and not the conclusion: presence_rollup_hour and presence_rollup_day carry counted and uncountable sample counts separately, min/max/mean over the counted ones alone, and no row at all for an hour nobody measured. A measured zero is a filled cell, a probe that got in and could not count is a hatched one, and an hour we never reached is the absence of a row -- the same three states the raw table keeps, after aggregation. The CHECK constraints refuse a bucket with a zero it did not measure and a bucket that claims to be a measurement of nothing. Partitions are now made ahead of need as well as before every append, so a month ending is never the first thing to discover a database we cannot write DDL to. The pass runs behind its own Postgres advisory lock, like the crawl loop -- CrawlLease is renamed AdvisoryLease, since two workers now share it. Retention is configuration, because 15.4 is open. It keeps everything at every grain by default, with 5.2's own figures a preset away, and it never runs ahead of the rollup: raw months go whole, and only once both grains have consumed them. 15.4 records the choice as ship conservative and tune. Salt rotation, which 11 promised and nothing implemented: a per-epoch HMAC salt derived from a deployment secret, an epoch label recorded beside every aggregate, and a rollup that refuses to combine an estimate across a rotation rather than publishing a bigger number. Names go in and sixteen bytes come out; nothing persists a name. Testcontainers 4.13 pulls SSH.NET 2025.1.0, which now has an advisory, and TreatWarningsAsErrors turns that into a broken restore for everyone. Bumped.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Member
Author
|
@coderabbitai review |
|
The daily rollup resumed from the hourly watermark, which is fine right up until the two statements do not both run. A pass that rolled the hours, committed their watermark and then died left the days unwritten -- and the next pass, reading the hourly mark, skipped everything older than the overlap, wrote its own watermark as though it had read it, and let retention drop the raw months behind it. Six days of history came back as six hour buckets and one day bucket, and then the raw rows went. The grain 5.2 keeps for ever, missing, with the only other copy deleted. Each grain now resumes from its own watermark and writes that watermark only after its own aggregation, so an interrupted pass resumes instead of skipping, and retention still waits for whichever grain is behind. An estimate with no salt epoch also cost more than itself: the invariant ran inside JSON deserialisation, so one such row threw out of the middle of a read and took the whole window with it, while the aggregation read the same row without complaint. The reader now drops the estimate and keeps the buckets, and the SQL filters the same case, so both paths agree that an unlabelled estimate is not one anything may compare with another. The maintenance service starts beside the crawler's migration run rather than after it, so it asks whether the schema is there instead of finding out as a 42P01 and standing down for five minutes. 11's salt machinery is in place and nothing feeds it: the WHO parser counts rows and never extracts a name, so no probe produces aggregates. Said plainly in 5.2 and beside the registration, rather than left to read as delivered.
Fifth and last link of this round: main → #27 → #32 → #31 → #21 → #28. Two conflicts, both in the crawler's options, and both purely additive: CrawlerOptions.Validate now validates the maintenance and salt settings this branch added and the submission settings #21 added, and the builder's Build carries all three through. Neither side was choosing between them. Everything else merged: the presence rollup registrations sit beside the claim, opt-out and submission registrations in one graph, and the two rotating salts are unrelated to each other — MUI.Crawl's hashes a player name for §11's aggregates, MUI.Crawler's hashes a submitter address for §7.6's per-source bound. Migrations arrive in chain order with nothing to renumber: 0009 opt-out, 0010 submitted games, 0011 presence rollup. This branch's edit to 0003 is a comment, so a database that has already applied it is unaffected.
Carries the passkey-endpoint test that landed on feat/claim-wiring down to the tip of the chain.
HarryCordewener
pushed a commit
that referenced
this pull request
Aug 15, 2026
Sixth link, and the first of round two: the chain now runs main → #27 → #32 → #31 → #21 → #28 → #30. MIGRATION RENUMBERED. 0009_slug_history.sql → 0012_slug_history.sql: #31 took 0009, #21 took 0010, #28 took 0011. MigrationRunner sorts embedded resource names ordinally and MUI.Catalog.csproj globs migrations/*.sql into LogicalName MUI.Catalog.Migrations.%(Filename)%(Extension), so a rename is the whole change — the file is still embedded and now sorts after 0011. Nothing in the tree referred to it by number. Six conflicts. PostgresData — #30 added the former-slug store with AddSingleton, into a method #32 had converted to TryAdd throughout precisely because AddMuiCrawler registers the same objects and one deployable calls both. The store is registered TryAdd for the same reason the availability store above it is: two of them would be two pools answering one question. Program.cs and SiteComposition — the same shape as #21's merge. #30 added UseMuiNotFoundPage and UseFormerSlugRedirects to a pipeline #32 had moved into UseMuiSite; they move with it, at the positions #30 put them in — the 404 page before the static files, the redirect after the account routes and before the route that would answer "not found". SiteHost now calls AddMuiSite/UseMuiSite instead of restating them, which its author flagged as a one-call follow-up. Verified it binds rather than merely compiling: commenting out either middleware inside UseMuiSite fails seven of #30's tests by name. ApiHost — #27 added an IGameQueries parameter and #30 added an Action<IServiceCollection> one, to the same optional slot. Both are kept and SlugRedirectTests names its argument. CrawlCyclePostgresTests took both new optional parameters, the spec's §10.1 took #27's prose with #30's paragraph in place of the "one gap remains" sentence that is no longer true — after this merge all three of the gaps that section lists are closed. And the compile breaks a restack earns: #30 gave ProbeIngestor a SlugMinter and CatalogueBinder an ISlugHistoryStore, which #31's and #21's Postgres tests construct by hand; Render's StubGameStore grew RenameAsync, throwing, because it throws for every other writer and a surface reaching for one is a surface in the wrong layer.
HarryCordewener
pushed a commit
that referenced
this pull request
Aug 15, 2026
Seventh link: main → #27 → #32 → #31 → #21 → #28 → #30 → #26. Two conflicts. Arguments.cs — this branch moved seed parsing out to CrawlSeed.Parse so the CLI, the environment variable and compose.yaml all read an address the same way; #31 added ParseAddress beside the ParseSeed that moved. Kept ParseAddress where it is and let ParseSeed stay gone: an opt-out address comes only from the CLI, and it takes an optional port, so it is not the same parse. Program.cs — the same shape as the two merges before it. This branch added configure.Apply(builder.Configuration) to the AddMuiCrawler call, which #32 had moved into SiteComposition.AddMuiSite; the line moved with it. That move earned a test. CrawlerSettings.Apply has thorough tests and every one of them calls it on a builder it constructed itself, so all of them pass on a site that never calls it — and the call has now moved between files during a restack, which is exactly how it would be lost. The new CompositionTests case resolves CrawlerOptions out of the site's own graph with Crawler:Enabled=false and a configured seed, and fails when the line is commented out. It also pins the half configuration may not do: a seed that arrives this way is never an operator seed, because §7.2's exemption is a claim a human makes about one address and an environment variable copied between deployments is not that human. Verified after the migration rename in the link below this one: all twelve migrations are still embedded in MUI.Catalog.dll and 0012_slug_history sorts last. This branch's new EnsureTheMigrationsWereFound target and the runner's empty-set refusal both still hold.
HarryCordewener
pushed a commit
that referenced
this pull request
Aug 15, 2026
Eighth link: main → #27 → #32 → #31 → #21 → #28 → #30 → #26 → #29. THE ANTI-FORGERY REORDER, WHICH WOULD HAVE VANISHED. This branch moves UseAntiforgery after UseAuthentication, and it was written against the Program.cs #32 deleted — so it merged clean and applied to nothing. The rule is reapplied inside the composition, and it is now a named method rather than three lines: UseMuiAntiforgeryAfterAuthentication. A method because the test that proves the rule matters built its own copy of the three lines. OwnerEndpointTests' harness said "Program's order" in a comment and restated it in code, so it asserted its own ordering and would have gone on passing with the site's reversed — which is the failure it exists to catch, one level up. The harness now calls the site's own method for the correct order and hand-builds only the wrong one, because there is no other way to build a thing that is not supposed to exist. Verified: swapping the two lines inside the method fails five of this branch's tests by name, and it failed none of them before. The ordering is worth that trouble. An anti-forgery token issued to a signed-in operator carries their identity; validating before authentication compares it against nobody and every owner's form post is refused as forged, while every public page — all GET — goes on working perfectly. PlainText — #27 factored the plain label into PlainText.Label, this branch added a third word to it at one of the four call sites. The three-way choice moved into Label: an owner's answer is owner-declared on the listing, on the game page and in the archive alike, and "declared" alone would put what an operator typed into our form and what their config file emits under one word. That was this branch's own argument for making the distinction at all; it just has one place to live now. app.css is two separate blocks, both kept. InMemoryFieldStore grew #30's LastChangedAtAsync, answering null — which is true of a store whose RecordChangeAsync already discards what it is handed. DeclaredOf's empty-value filter survived intact, and the ladder conflict its author expected with #28 did not happen: #28 never touched NpgsqlGameQueries. A cleared owner row is still filtered before the ladder rather than after it, so it cannot win its group and silence the MSSP value underneath.
HarryCordewener
pushed a commit
that referenced
this pull request
Aug 15, 2026
Ninth link, and the first of the three that were based on feat/claim-wiring rather than on main: main → #27 → #32 → #31 → #21 → #28 → #30 → #26 → #29 → #33. Two conflicts, and one of them is the reason these three were flagged. Account.razor — this branch put the scorecard link in the <li> a claimed game used to be, and #29 has since turned each claimed game into a <details> with an owner panel inside it. Textually the two edits are the same lines; what the branch meant was "one link per verified claim, on the dashboard and nowhere else", so the link moved into the <summary> beside the game's name and the verified date. Taking either side wholesale would have lost the owner panel or lost the only route to the scorecard. app.css is two independent blocks, both kept. Read the branch's tests rather than trusting them: MsspLintTests and MsspScorecardSurfaceTests assert MsspLint's judgements and that the page is CLOSED to a stranger, neither of which touches the dashboard's markup, so nothing here went stale. They also do not cover the dashboard link itself — noted rather than fixed, because Account.razor has no render harness and building one for one anchor is a bigger change than this merge.
HarryCordewener
pushed a commit
that referenced
this pull request
Aug 15, 2026
Tenth link: main → #27 → #32 → #31 → #21 → #28 → #30 → #26 → #29 → #33 → #34. THE BADGE WAS PUBLISHING A GAME'S OWN ASSERTION AS OUR MEASUREMENT, and this merge is where that became visible. This branch added GameSummary.PlayersNowAt — "when the count was measured" — and said so in its own doc comment: "not the whole of §10.1's fix, the codebase still has no chip". #27 landed the whole fix while this branch sat on feat/claim-wiring, so the summary now carries PlayersNowProvenance, a chip with the count's source on it. PlayersNowAt is dropped and the badge reads the instant off the chip; keeping both would have been two answers to one question, which is the thing #27 exists to stop. But an instant is not the interesting half. PlayersNow can come from MSSP PLAYERS — PresenceChoice ranks WHO, then MSSP, then the connect screen — and this badge writes "N players measured 4m ago", returns state "measured" in its JSON, and paints the accent that means measured on every other surface here. On somebody else's front page, where we cannot correct it. That is rule 5 broken by a format string, which is very nearly the sentence §10.1 uses about the unlabelled listing. So Counted now requires ProvenanceChip.IsMeasured — the same predicate ApiMapper.Counted already uses to decide playersNowState, so the badge and /api/games/{slug} cannot disagree about one game. A declared count reads as unknown: three states, no new vocabulary, and the badge says only what we measured. What the game says about itself is on its page, attributed. Nothing covered this: BadgeApiTests used ashen-court only to assert two URLs, and ashen-court is the fixture's MSSP-declared row — put there by #27 precisely as the argument for labelling. ADeclaredCountIsNotPublishedAsA- MeasuredOne now pins it, with m-u-s-h as the measured control, and it fails if the IsMeasured guard is relaxed. Account.razor for the third time: the badge snippet was written against the <li> a claimed game used to be. It is now in the <details> body beside the owner panel rather than in the <summary>, because a <details> nested inside a <summary> is interactive content inside a control. NEEDS A HUMAN: ProvenanceChip.IsMeasured is Handshake-or-Who, so a count read off a connect screen is "declared" — while migration 0003 says in terms that a banner count "is still a measurement of ours". The two disagree, the disagreement predates this chain, and it now decides whether Aardwolf's badge shows a number. I have kept the badge consistent with the API rather than picking a side.
HarryCordewener
pushed a commit
that referenced
this pull request
Aug 15, 2026
Eleventh and last link of the restack: main → #27 → #32 → #31 → #21 → #28 → #30 → #26 → #29 → #33 → #34 → #35. MIGRATION RENUMBERED. 0012_claim_intent.sql → 0013_claim_intent.sql, since #30's slug history now holds 0012. Thirteen migrations are embedded in MUI.Catalog.dll, 0013 sorts last, and nothing referred to it by number. Three conflicts and one clean merge that did not compile. Account.razor, for the third merge running, and this one had the most in it. The dashboard's status banner is now one else-if chain — resigned, then saved, then refused — because a redirect carries exactly one outcome and two banners at once would be two answers to one action. A claimed game's block holds, in the order an owner would want them: who else owns it, the enrichment panel, the badge snippet, the history, and giving up the claim last. #35 was written when a claimed game was an <li>, so all of it had to move inside the <details> #29 introduced; the co-owner line in particular was inside the <summary>, which is not somewhere a <p> may live. Passkeys.cs maps both write surfaces rather than choosing: MapMuiOwnerWrites is §8.5's enrichment and §11's suppression, MapMuiOwnership is §8.4's counter-claim and resignation. They are different routes. Claim.razor merged clean and broke the build, which is the useful kind of failure. #21 changed this page from IGameQueries to IGameStore on purpose — a submitted game is hidden from the public read until somebody claims it, so looking it up through the listing's own query made claiming the one thing a hidden game could never do — and #35 added three uses of the old Page.Summary against the view model that is no longer loaded. They now read the row, which is what the rest of the page already did. Read #35's tests rather than trusting them, as asked: OwnershipPostgresTests and OwnershipSchemaTests assert ClaimService and the claim_intent schema against a real database, neither of which touches the dashboard markup or the page's lookup, so nothing in them went stale. The dashboard markup itself has no render harness on any of these three branches.
Carries the shared measured/declared predicate and /submit's opt-out refusal down the chain.
This was referenced Aug 15, 2026
HarryCordewener
added a commit
that referenced
this pull request
Aug 15, 2026
The rollup #28 built was written and never read. PresenceMaintenance filled presence_rollup_hour every pass and the only consumer of NpgsqlPresenceRollupStore was the writer; the heatmap still selected straight out of presence_sample. So §5.2's whole arrangement — roll the hours up, keep them for two years, let retention drop the raw months underneath — bought nothing, and PresenceRetentionOptions.AsDesigned was a setting that could only ever lose data the grid still depended on. The grid now reads across the rollup watermark: rolled-up hours below it, raw samples above it. Both halves are needed and neither is optional. - Below the watermark the raw partitions are droppable by design, and reading raw alone loses the far end of every heatmap the moment a deployment configures retention at all. - Above it there is nothing else to read. The rollup only ever consumes whole elapsed hours, so the newest hours are permanently ahead of the watermark, and reading the rollup alone would render a probe taken ten minutes ago as an hour nobody measured — §5.4's worst collapse, on the part of the grid a reader looks at first. No watermark means nothing was ever rolled up, and the boundary falls to -infinity, which is exactly the read this was before. The tally is summed rather than averaged across buckets, so an hour probed once does not outweigh an hour probed twelve times. Rankings and the digest still read raw, correctly: the rankings median needs the distribution, which a rollup keeping min/max/sum/mean cannot answer, and both work inside windows far shorter than any retention floor. HeatmapWindow's rationale said the site reads raw for the graphic, which is now false; it says what actually holds the floor up instead. ActivityWindow was a second hand-written 56 days beside it and is now the same constant. 1212 tests across five suites, Postgres exercised. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
presence_samplehas been RANGE-partitioned monthly since migration 0003 "so §5.2's retention and rollups can later work on whole partitions". Later is now.What this adds
Rollups (§5.2).
presence_rollup_hourandpresence_rollup_day, aggregated on a schedule from the raw table, pluspresence_rollup_state— the watermark saying how far each grain has consumed it.The hard part is §5.4, and it is the thing the tests are about. A bucket carries a tally, not a conclusion:
counted_samplesandunmeasurable_samplesare separate,min/max/meanare over counted samples alone and are NULL when there were none, and an hour nobody measured produces no row at all. So after aggregation a measured zero is still a filled cell, "probed and could not count" is still hatched, and "not measured" is still the absence of a row. FourCHECKconstraints refuse a bucket carrying a zero it did not measure and a bucket that claims to be a measurement of nothing.The mean is stored as
sum/countrather than as an average, so a day built from hours with different numbers of probes is exact rather than an average of averages.Partition maintenance. Partitions are now created ahead of need (two months by default) as well as before every append, so a calendar rollover is never the first thing to discover a database the pass could not reach. The whole partition lifecycle — name, make, list, drop — lives in
NpgsqlPresenceStore/PresencePartitions, because two places that named a month differently would drop a month neither meant.Scheduling.
PresenceMaintenanceServiceis aBackgroundServicegated on a Postgres advisory lock with its own key, following the crawl loop's pattern: N web replicas run one pass.CrawlLeaseis renamedAdvisoryLeasenow that two workers share the mechanism (AdvisoryLease.CrawlKey,AdvisoryLease.PresenceMaintenanceKey). It is registered independently ofCrawlerOptions.Enabled— a pure web replica still wants next month's partitions to exist — and it asks whether the schema has been applied yet rather than discovering it as a42P01, because it starts beside the crawler's migration run rather than after it.Retention (§15.4, open).
PresenceRetentionOptions, the same shapeDatasetLicenceOptionsuses for the licence question, and the default keeps everything at every grain. §5.2 does authorise dropping raw samples after aggregation, but the period has never been checked against a deployment's storage and §15.3 (the cost envelope that bounds it) is open too — so §5.2's figures ship as the named presetPresenceRetentionOptions.AsDesignedinstead of as the default. Raw rows go by whole partitions, never row by row. Each grain resumes from its own watermark and commits that watermark only after its own aggregation, and every retention cutoff is clamped to the older of the two — so a pass that rolled the hours and then died resumes rather than skipping, and cannot drop what the daily grain has not read.Salt rotation (§11).
RotatingSaltProvider: per-epoch salt = HMAC(deployment secret, epoch label), so replicas agree without anything storing a salt, and an unconfigured deployment gets a per-process random one (a weaker estimate, never a reproducible salt). The epoch label is recorded beside every aggregate —PresenceAggregatesrefuses an estimate that does not name its epoch — and the rollup carries an estimate only where a bucket's samples share one epoch. An unlabelled estimate is dropped by the reader and filtered by the aggregation, so both paths agree about what is legible, and one bad row costs its own estimate rather than the window it is in.Nothing feeds the salt yet, and the spec now says so. The
WHOparser counts rows and never extracts a name, so no probe produces aggregates andPresenceAggregatesis constructed nowhere insrc/. The columns, the epoch branches and their constraints are unreached in the shipped pipeline. That is the safe half to have built first — the rule that names are never persisted is kept by there being no path that handles one — but the unique-player estimate is designed, not delivered, and §5.2 says that in as many words rather than letting the hunk read as shipped.Spec. §5.2's "rollups are not built and have no owner" is replaced by what is now built and what is not (the heatmap still reads raw; §11 is unfed), and §15.4 records the decision as ship conservative and tune, the way §15.5 already does.
Verified
dotnet build MUIndex.slnx -c Releaseclean, then each suite directly against Postgres via Testcontainers on Podman:The three-state test seeds a counted hour (including a measured zero), an hour probed twice and uncountable both times, an hour with nothing in it and an hour with a lone measured zero, and asserts all four come back distinguishable — plus two raw
INSERTs proving the schema refuses to hold a zeroed hatch or an all-zero row.Second commit adds four more, each red before its fix: the interrupted pass that produced six hours of history and one day of it; retention refusing to drop while a grain is behind; an unlabelled estimate that no longer takes its window down and no longer reaches the rollup's peak; and a service started against an empty database that waits for the migrations instead of failing on them.
Notes
mainis at 0008 and PR A stranger may hand us an address, and an address is all we take #21 uses 0010; this takes 0011 and expects to be renumbered at merge. Until then an existing database applies 0009/0010 after 0011 while a fresh one applies them before, which is the merge-time fix rather than something this branch can settle alone.Testcontainers.PostgreSql4.13.0 → 4.14.0: 4.13 pullsSSH.NET2025.1.0, which has since picked up GHSA-q939-rpr3-3284, and withTreatWarningsAsErrorsthat fails restore for everyone. 4.14 takes SSH.NET 2026.0.0.🤖 Generated with Claude Code