[ConfigManager] Install sei.toml for Looked-Up Keys 1/3 - #4043
Draft
bdchatham wants to merge 2 commits into
Draft
Conversation
…oks up A node's app.toml settings are read one key at a time, when the thing that wants them asks, and that happens after the boot has built the source they are read from. So a value can be put into that source and the later lookup finds it. This installs the declared keys an operator's sei.toml supplied, and nothing else. Only what a source supplied, not the whole resolution. Resolve answers for every declared key, so installing all of it would write this binary's own defaults over an operator's file for every key they did not mention. The keys of a section whose reader decodes its file whole are skipped, because putting a value into the source is no delivery at all for those: their file is read into a struct before this runs and nothing consults the source for them afterwards. They are marked here as needing that second delivery, which is a separate change. Nothing here can stop a node starting, and the guard that makes that true is new. What follows walks the node's own configuration types by reflection and decodes through two libraries, so a panic is a shape nobody predicted rather than a value an operator wrote, and letting it escape would refuse a boot for the one reason this path promises never to refuse one. An unreadable sei.toml is no longer reported as an absent one. A node with no such file is every node today, so that stays quiet; a node whose file will not parse, or records a schema this binary does not know, or names no node kind, is a node where somebody wrote the file and it is doing nothing. Collapsing the two meant the only signal an operator had for their mistake was the one that got collapsed. Verified by mutation: collapsing every read failure back to absent fails the new distinction, and removing the guard lets a panic escape the install path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4043 +/- ##
==========================================
- Coverage 61.25% 60.27% -0.98%
==========================================
Files 2153 2056 -97
Lines 188426 176963 -11463
==========================================
- Hits 115412 106671 -8741
+ Misses 62269 60499 -1770
+ Partials 10745 9793 -952
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
Author
|
@seidroid review |
…ile can cost An operator was told their sei.toml wrote keys it does not contain. Every unmatched flag name reached the resolution under its own name, and the resolution merged the file's undeclared keys with the flags' before reporting them, so `--home` and `--trace` were reported as the file's mistakes on every boot. That is the only signal there is for a mistyped key, and it fired whether or not one was typed. Resolved now carries UnknownInFile and UnknownFromFlags apart, and the install reports only the file's own. The environment needs no set of its own: it is asked only for names derived from declared keys, so it cannot carry one that is not declared. Ignored carries its reason. The resolver already built a reason per key and threw it away, so the one warning an operator gets for an ignored variable told them nothing about why. The reason now travels with the key, and a parallel mechanism for recording the same fact per section is deleted: it had no callers, nothing read what it recorded, and the package already states that the reason belongs to the channel rather than to any section. A misspelled delivery declaration is reported. A section states its keys and how they are delivered from two calls side by side, and only the first was checked, so `memool` beside a section registered as `mempool` left the real section's keys installed into a source its reader never asks. Derived at every read, because nothing fixes the order of the two calls. One acquisition where there were two. The sections and the delivery declarations are halves of one answer, and read separately a section arriving between them is described by one half and absent from the other. Reset clears the declarations too, so a fresh registry cannot hold a declaration naming a section it does not have. A file is read within a bound. A 200 KB sei.toml of one deep heading cost 7 GB and a 400 KB one killed the process, on every restart, and a recover cannot catch that. Size, key depth and array nesting are now bounded before the bytes are parsed, and the refusal that names an over-deep key no longer renders the whole key: that message was 400 KB for a 200 KB key. The unbounded reports are bounded, the install names the keys a node reads here for the first time, and the package documentation describes the delivery that exists. 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
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.
Installs the values an operator's
sei.tomlsupplies, for the keys a reader looks up one at atime. 1516 lines. First of three: the second delivery and the
checkcommand follow separately.Why a value can simply be installed
A node's
app.tomlsettings are read per key, when the thing that wants them asks, and thathappens after the boot has built the source they are read from. So putting a value into that
source works: the later lookup finds it.
Only what a source supplied
Resolveanswers for every declared key. Installing all of it would write this binary's owndefaults over an operator's file for every key they did not mention, so only the keys something
other than the defaults supplied are installed.
The keys this deliberately does not install
A section whose reader decodes its file whole is skipped. Putting a value into the source is no
delivery at all for those: their file is read into a struct before this runs, and nothing
consults the source for them afterwards. They are marked here as needing a second delivery,
which is the next change in the series.
That marking is walked from what the registrations recorded rather than a list beside them, so a
section cannot be registered without being delivered.
Nothing here can stop a node starting
Selecting this manager is a switch rather than a configuration change: a node with nothing
written, or something wrong written, starts exactly as it did.
The guard that makes that true is new. What follows walks the node's own configuration types by
reflection and decodes through two libraries, so a panic is a shape nobody predicted rather than
a value an operator wrote, and letting one escape would refuse a boot for the single reason this
path promises never to refuse one. An existing comment already claimed this property; it was not
true until now.
An unreadable file is not an absent one
A node with no
sei.tomlis every node today, so that case stays quiet. A node whose file willnot parse, or records a schema version this binary does not know, or names no node kind, is a
node where somebody wrote the file and it is doing nothing. Those were collapsed into the same
answer, which meant the only signal an operator had for their mistake was the one that got
collapsed.
What review changed
Reports that named the wrong thing. An operator was told their
sei.tomlwrote keys it doesnot contain: every unmatched flag name reached the resolution under its own name, and the report
merged the file's undeclared keys with the flags'. So
--homeand--tracewere reported as thefile's mistakes on every boot, which is the one signal there is for a real typo. The resolution now
keeps the two apart and the report names only the file's. In the same family, an ignored
environment variable now carries its reason: the resolver already built one per key and threw it
away, so the warning an operator got said nothing about why their variable did nothing. A second
mechanism for recording that same fact per section is deleted, because it had no callers, nothing
read what it recorded, and the reason belongs to the channel rather than to any section.
A guard that measured nothing. A section states its keys and how they are delivered from two
calls side by side, and only the first was checked against the struct, so
memoolbeside a sectionregistered as
mempoolproduced no complaint and left the real section's keys installed into asource its reader never asks. That is now reported, derived at every read because nothing fixes the
order of the two calls. Two reads of the registry that had to describe the same registry became
one, and
Resetclears the delivery declarations as well as the sections.What a file can cost. A 200 KB
sei.tomlholding one deep heading cost 7 GB of memory, and a400 KB one killed the process, on every restart until somebody deleted the file. A recover cannot
catch that, so size, key depth and array nesting are now bounded before the bytes are parsed. The
refusal that names an over-deep key no longer renders the whole key: that message was 400,306
bytes for a 200 KB key and is now 386.
The reports that could grow without limit are capped, the install names the keys a node reads from
this registry for the first time, and the package documentation describes the delivery that exists.
Notes for review
Formatters,
go vetandgolangci-lintclean;-count=2 -shuffleclean.Mutation-verified: collapsing every read failure back to absent fails the new distinction, and
removing the panic guard lets one escape the install path. The second was worth checking rather
than assuming, since that test passed the moment it was written.