fix(nextly): one predicate every cache asks, and an epoch that identifies its own store - #1847
Conversation
…fies its own store Round five is three findings, and the first is a regression the round before introduced. ## A batch stopped reaching a cache it never owned Deferring the announcement to the batch's exit was right, and the local retirement that went with it empties the maps this module holds. A key's copied grants are not one of those: they live in another module, so no list of maps to clear was ever going to reach them, and a key went on answering with revoked grants for the batch's whole length — unbounded for a seeder. Adding a fourth clear would have been the same mistake with a longer list. What every tier already asks is `stampIsCurrent`, so the batch's untrusted state moved INTO it: `retirements` lives in the epoch module beside the trust it decides, `duringRetirement` scopes it, and a batch holds one open across its whole body. `resolvedUnderCurrentRevision` is now nothing but `stampIsCurrent` — the flush depth it used to add was exactly the thing the other tiers could not see, and `permissionFlushDepth` is gone. Scoped rather than begin-and-end so a throw cannot leak it, and a leaked one would leave the install permanently uncached. ## A write that landed but was never read back looked settled If the upsert succeeds and the read that follows it fails, the count is clear — the row really did move — while this process still holds the value from BEFORE the write. Every stamp comparison then starts passing again, against a stamp that predates the change, and a check that began before a revocation can finish after it and refile the old answer. Trust needs both halves, so `observed` is tracked as its own fact rather than inferred from the backlog being empty: cleared when a write lands, set when a read succeeds, and `epochIsTrustworthy` requires it. ## An empty store had no identity A table with no row answered `:0`, which is what EVERY never-yet-invalidated store answers — so a failover onto a different one, or a restore from a backup taken before the first role change, left every cached answer looking current. That is precisely what `generation` exists to catch, and epoch zero is where a fresh install sits for longest. The row is created rather than imagined, and read back. Adding zero is the same statement the bump path makes, so it IS that statement: `raiseSharedRevision(by)` now serves both, and the row is created with an identity when absent and left alone when present, decided by the database rather than by a check-then-insert two instances can both win. `selectSharedRow` is the other half, so the read has one spelling too. Where the create does not take, the answer is still zero and still not a failure: a table that exists with nothing in it has never invalidated, and refusing an authorization check over it would be worse than the staleness.
|
Warning Review limit reachedNext included review available in 28 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review Follow-up to #1829, which merged before this round's findings were worked. All three are fixed here. The one worth the closest look is the first: the batch's untrusted state moved INTO Also worth checking: |
|
@coderabbitai review |
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f50806be48
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
@nextlyhq/blocks-react
@nextlyhq/builder
create-nextly-app
@nextlyhq/eslint-plugin
nextly
@nextlyhq/plugin-form-builder
@nextlyhq/plugin-mcp
@nextlyhq/plugin-page-builder
@nextlyhq/plugin-sdk
@nextlyhq/plugin-seo
@nextlyhq/storage-s3
@nextlyhq/storage-uploadthing
@nextlyhq/storage-vercel-blob
@nextlyhq/ui
commit: |
`observed` records whether the epoch this process answers with came from the row, and it was cleared only where a bump had been sent. The seed path did not clear it, so a process holding one store's identity that failed over to a store with no row kept every cached stamp trusted: the insert that would have established the new identity is exactly what a read-only credential rejects, and a rejected insert reaches the catch having changed nothing. The stale stamp then matched for the life of the process, which is a revoked grant still being served and the generation check defeated by the case it exists for. Trust now drops the moment the row is found missing, ahead of the write, and comes back only where a row was actually adopted. The read-back failing after an accepted insert lands in the same place, so both doors are shut rather than the louder one. A store that accepts the create and stays empty still answers zero rather than refusing an authorization check, but it says so: the warning names which of the two situations it is, because a missing table is what `nextly db:sync` reconciles and a row that will not persist is a permission or a storage problem no schema command can fix.
f50806b to
3895fe3
Compare
💡 Codex Review Report seed failures as row-establishment failures
When the initial select succeeds with no row but either ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3895fe3b46
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The two degraded reports exist because the remedies differ, and the report was chosen in the catch, which cannot see which of them applies. A store answering reads and rejecting the create is the read-only credential case: the table is demonstrably there, and the operator was sent to `nextly db:sync`, the one remedy that cannot help. They would run it, see nothing change, and have no reason to look at the credential. The empty-row path now marks itself as entered, so a failure from that point on is reported as a row that would not persist. Both of its failures are covered: the create rejecting, and the read back after an accepted create failing. The control holds the other direction, where a table that cannot be read at all is still the schema's to reconcile.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ac8c4af67
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function reportDegraded(message: string, detail?: string): void { | ||
| if (degraded) return; |
There was a problem hiding this comment.
The rejected-seed case is now classified correctly in isolation, but this single degraded flag suppresses a later, different diagnosis. If a process first starts before db:sync, then the table is created while its runtime credentials remain unable to establish the row, subsequent refreshes reach ROW_NOT_ESTABLISHED but emit nothing because the earlier table warning set this flag. The operator therefore sees only the obsolete db:sync remedy. Track the reported degradation category (or clear it once the table is readable); the new diagnostic test currently resets module state between failure modes, so it cannot expose this transition.
AGENTS.md reference: AGENTS.md:L228-L233
Useful? React with 👍 / 👎.
1 participant
Follow-up to #1829, which merged before this round's findings were worked. Three corrections to the shared RBAC epoch, and each is a cache that went on answering after the answer stopped being true.
A batch stopped reaching a cache it never owned
#1829 deferred the shared announcement to a batch's exit, which was right, and emptied the caches
permissions.tsholds. An API key's copied grants are not one of those. They live in another module, so no list of maps to clear was ever going to reach them, and a key went on answering with revoked grants for the batch's whole length — unbounded for a seeder.Adding a fourth clear would have been the same mistake with a longer list. What every tier already asks is
stampIsCurrent, so the batch's untrusted state moved into it:retirementsnow lives in the epoch module beside the trust it decides,duringRetirementscopes it, and a batch holds one open across its whole body.resolvedUnderCurrentRevisionis now nothing butstampIsCurrent. The flush depth it used to add was precisely the thing the other tiers could not see, andpermissionFlushDepthis gone.Scoped rather than begin-and-end so a throw cannot leak it — a leaked one would leave the install permanently uncached.
A write that landed but was never read back looked settled
If the upsert succeeds and the read after it fails, the count is clear — the row really did move — while this process still holds the value from before the write. Every stamp comparison then starts passing again, against a stamp that predates the change.
Trust needs both halves, so
observedis its own fact rather than inferred from an empty backlog: cleared when a write lands, set when a read succeeds.An empty store had no identity
A table with no row answered
:0— what every never-yet-invalidated store answers. So a failover onto a different one, or a restore from a backup taken before the first role change, left every cached answer looking current. That is exactly whatgenerationexists to catch, and epoch zero is where a fresh install sits for longest.The row is created rather than imagined, and read back. Adding zero is the same statement the bump path makes, so it is that statement:
raiseSharedRevision(by)serves both, and the row is created with an identity when absent and left alone when present — decided by the database rather than by a check-then-insert two instances can both win.Evidence
4 mutation controls, all red, each killing exactly the case named for it:
stampIsCurrentstops asking about retirementsserves nothing while a retirement is runningstays untrusted when the write landed but the read did notgives a store with no row an identity instead of a shared zero(+ the different-store control)loses it inside a batch, which announces nothing until it exitsThe identity cases needed a new fake that actually holds the row: the existing one reports writes as successful without storing anything, so the seeding was unassertable against it — the assertion would have been satisfied by a state that predated the change.
Gates: 950 files / 12,148 unit, 12 files / 95 integration, types, lint, comment convention,
fallowzero introduced, changeset covers all 26 lockstep packages (verified with a control that removing one package makes the check refuse).One unrelated commit, and why it is here
mainis currently red on the comment-convention gate for every open branch: a comment merged with #1834 names the pull request it arrived in. It is one line in a test, the behaviour is untouched, and leaving it would have meant opening this PR into a red gate that is not mine. Called out here rather than buried.