Publish immutable config snapshots; atomic settings persistence, timing validation, payment/build-retry fixes - #172
Open
pk910 wants to merge 5 commits into
Open
Publish immutable config snapshots; atomic settings persistence, timing validation, payment/build-retry fixes#172pk910 wants to merge 5 commits into
pk910 wants to merge 5 commits into
Conversation
…ing shared config in place The settings service now swaps a fresh Config generation atomically on every applied change (atomic.Pointer + shallow copy); modules hold *config.Service and load one snapshot per operation instead of reading a shared mutable *config.Config. Fixes the torn-string-read race reachable through the unauthenticated WebUI/API port (reported in #161), including the JSON-marshal paths (GET /api/config, SSE config event), and makes multi-field reads coherent within one settings generation.
Ports #169 onto the config-snapshot model: - db.PutSettings upserts a whole batch in one transaction (replaces the per-row PutSetting); the constructor's CLI reconcile batches too - SetMany validates the batch's resulting config against ValidateTimingBounds (inverted bid window, negative / past-deadline reveal time), persists the whole batch durably, and only then applies and publishes the new snapshot — a failure anywhere leaves memory and the state-db untouched and is reported to the caller instead of the previous unconditional success - startup validates operator timing config hard; a persisted override that violates the bounds (saved by an older release) is kept but warned about - per-slot action-plan overrides deliberately stay free of these bounds: chaos scenarios belong in plans, not the global baseline Co-authored-by: Damilola Edwards <damilolaedwards@users.noreply.github.com>
Ports #170 onto the per-key payment tracker: a reveal completing before the won bid is recorded (possible whenever a head event is delayed past the reveal gate, and routine in the Builder API flow, which requests the reveal already at block submission) used to silently drop the balance deduction and leave an orphaned pending payment. MarkRevealed now records such slots per key in earlyReveals; RecordWonBid applies the deferred deduction immediately instead of creating a pending entry. Stale markers prune on the same two-epoch schedule as pending payments. Co-authored-by: Damilola Edwards <damilolaedwards@users.noreply.github.com>
Ports #171: executeCandidateBuild marked a (slot, parent-tuple) candidate as started before attempting the build but never cleared the marker on failure (engine call or payload transform), so a single transient error silently dropped every retry of that candidate for the rest of the slot — including a CL-client attributes redelivery for the exact same parent. Both failure paths now clear the marker. Co-authored-by: Damilola Edwards <damilolaedwards@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
SummaryThe PR converts the settings service to immutable atomic config snapshots (fixing the #161 torn-read race across all field types and the JSON/marshal paths), makes SetMany batches durably atomic via a single DB transaction with pre-commit timing validation, and adds an early-reveal payment-accounting fix plus a build-retry fix. The migration is thorough and correct: no post-publication mutation of snapshots, no leaked locks, and the retry/payment fixes are concurrency-safe. One low-severity concern remains around the interplay of keep-but-warn persisted overrides and whole-config timing validation. Issues
Reviewed @ |
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
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.
Immutable config snapshots
The settings service no longer mutates one shared
config.Configin place. It now publishes immutable snapshots: every applied change builds a freshConfiggeneration (shallow copy — all fields are values) and swaps it in atomically viaatomic.Pointer. Modules hold the*config.Serviceinstead of a raw*config.Configand load exactly one snapshot per operation (HTTP request, scheduler tick, build, reconcile pass), threading it down the call stack.This fixes the data race reported in #161 — a UI/API settings write racing an unsynchronized hot-path read could produce a torn string read (a remote crash primitive, since the write path is unauthenticated by default on
--api-port) — but fixes it for all field types at once, including the JSON-marshal read paths (GET /api/config, the SSEconfigevent) that a field-level fix could not cover. As a bonus, multi-field reads are now coherent within one settings generation (e.g. a plan freeze can no longer observe half of aSetManybatch).Design notes:
*config.Servicemay live in struct fields;*config.Config(and section pointers) appear only as function parameters and locals — storing a snapshot freezes that consumer on a stale generation.Configstays a plain marshal-able struct: viper/YAML loading,json.Marshal, and test construction are unchanged; no mutexes inside config structs (no copylocks hazards).Fieldregistry unchanged —recomputepoints the existing closures at the staging copy before publishing.config.NewStaticService(cfg).cmd/run.go(its fleet targets are mutable settings);payload_builder.GetConfig()now returns the latest snapshot; the no-opUpdateConfigwas removed.SetManybatches against concurrent readers and asserts snapshot coherence (not just detector silence).Included work from other PRs
copylocks.db.PutSettingsbatch-transaction upsert;SetManynow validates the batch's resulting timing invariants (ValidateTimingBounds: inverted bid window, negative / past-deadline reveal time), persists durably, and only then applies and publishes — a failure leaves memory and state-db untouched and is reported to the caller. Startup validates operator timing config hard; a persisted override violating the bounds (saved by an older release) is kept but warned about. Per-slot action-plan overrides deliberately stay free of these bounds — chaos scenarios belong in plans, not the global baseline.MarkRevealed/RecordWonBidorder independence, reimplemented for the per-keyPaymentTracker(the PR predated the key-fleet work and no longer applied). An early reveal defers its deduction per key; the late won-bid report applies it immediately instead of orphaning a pending payment for two epochs.executeCandidateBuildclears the per-candidatestartedmarker on both failure paths, so a transient engine/transform error no longer blocks every retry of that (slot, parent-tuple) for the rest of the slot.Testing
go build ./...,go vet ./...(clean), and the fullgo test -race ./...suite pass after every commit. Regression tests are included for each ported fix, each verified to fail against the pre-fix code.