chadtmiller.com/knotless

Switch repository

Loading…

What changes

A write to one record patches what is kept. Index::assembled kept a repository's pulls and issues, but any write about the repository threw all of it away, and the next page read every record again and assembled them. Now a write of one pull, issue or comment (or its deletion) notes just that record's URI against the repository (Stamps::mark_item), and the next read reads those rows again by key, replaces them in the kept set, applies the owner's hides and blocks with forge::moderate, and assembles again. The kept set keeps its first read time, so the minute's expiry still holds.

What still reads the repository whole. More than 32 records written since the kept set was made, a write of the repository's own record, a hide or block (or taking one back), a takedown, a label, a move, an account forgotten, and the minute passing.

Why

On prod, Index::forge's read of a repository's records averaged 148 ms over 1,178 calls in the stats window (max 2,157 ms), and assembled missed its cache on 298 of 1,736 lookups (17%) in the last 200 cache summaries, nearly all from writes to busy repositories (comments, check-triggered edits). Each miss paid that read plus the JSON parse of every record.

Alternatives weighed. Keeping per-item assembled results would skip the assembly too, but an item depends on its comments, reviews, labels and hides, so it needs a dependency graph that moderation can break; a patched record set is simple and cannot disagree with the whole read, which the test checks. Keying stamps by record only (no whole-repository stamp) would lose the safe fall back for hides, blocks and moves. This keeps the whole stamp for everything whose reach is wide, and uses record stamps only where a record changes only itself. The read of the touched rows is a primary key lookup per URI (json_each joined to forge.uri), so it scales with the writes, not the repository, and the plans check covers it.

How it was checked

A new test patches an edit, an add, a comment and deletions into a kept set and holds the result equal to Index::forge each time, and covers a hide, a hide taken back, an author gone inactive, another repository's write, and a flood past the limit. A second tests the stamps. The existing kept-until-written test is unchanged. Everything else is left to CI.

Shortcomings

The assembly itself still runs over all records after a patch; only the SQL read and parse are saved. If after numbers show assembly dominating, assembling per item is next. Not measured after deploy yet.

Changeset: none

🤖 Generated with Claude Code

This patches only the records written since into a repository's kept set, instead of reading it whole. The stamps design and the test against Index::forge look sound. I found one race that can leave a write out of the kept set for up to a minute.

In assembled, the fresh check and self.stamps.touched(scopes, at) run before let at = self.stamps.now(). Say a comment's mark_item lands after touched was computed but before now() is taken. Its stamp is ≤ the new at, and it is not in touched, so patched never reads that row. The new Made then carries an at past the stamp, so since(scopes) <= at holds and the kept set looks fresh. The write is invisible until KEPT_FOR expires. The same goes for a hide, block or other whole-repository mark that lands in that window. A whole read doesn't have this problem, because its read happens after now().

The fix is to take at = self.stamps.now() first. Then run the fresh check and touched against the kept set, and read after that. Any write after at then stamps past what is kept and is seen on the next read. A write between the old m.at and the new at is in touched, or makes the set stale, so it is read.

The test doesn't reach this, since it is single-threaded.

The race from my earlier review is fixed. assembled now takes at = self.stamps.now() first, before the fresh check, touched and any read. A write that lands after that point stamps past at, so the next read sees it as touched or stale. A write between the old m.at and the new at is either in touched or makes the set stale, so it gets read. A write that lands during the read may be read early, and it is read again next time. That is harmless. The change since the last round is only this reordering, and the rest of what I looked at before stands.

My earlier approval stands. The only change since it is that the kept set now stores the owner's hide entries, which moderate takes out of the records. patched adds them back before moderating again, so a record that was re-read is checked against the same hides as in a whole read. Without this, a patched set would have lost the hides, because the old entries came from a list that was already moderated.

I did not check that moderate accepts input that has already been moderated, or that the /{nsid::HIDE}/ URI match picks out exactly the hide records. The pull's equality test against Index::forge covers a hide and a hide taken back, so a mismatch there should show up in CI.

My earlier approval stands. The only change since it is a rustfmt-style reflow of the tuple built from the kept set in assembled. It doesn't change behaviour. The test and check runs were still queued when I looked, so CI has the final say on the equality test.

My earlier approval stands. The only change since it is in the test. The owner now blocks did:e, and the test then writes an issue from that account. The equality check against Index::forge must still hold, so the patch path has to leave out a blocked account's write, as the whole read does. This adds coverage and changes no behaviour. CI has the final say on the test.