Skip to content

refactor: cache config reads and lock read-modify-write in pkg/config - #360

Open
hanthor-hive-agent[bot] wants to merge 4 commits into
mainfrom
arch/config-cache-and-lock
Open

hanthor-hive-agent[bot] wants to merge 4 commits into
mainfrom
arch/config-cache-and-lock

Conversation

@hanthor-hive-agent

Copy link
Copy Markdown
Contributor

Refactor

pkg/config had no in-memory representation of the loaded config: every
read helper (Contexts, Peers, DefaultContext, DefaultBackend,
HasBackend, Folders, LibvirtURI, IncusRemote, ...) called Load("")
independently, each paying a fresh os.ReadFile + yaml.Unmarshal.
pkg/web's request handlers chain several of these per request
(handleListVMs alone calls Peers() and DefaultContext(), which itself
calls Contexts() — that's at least three independent file reads for one
logical "give me the current config" need).

Every mutating helper (AddContext, RemoveContext, SetFolders,
SetDefaultContext, SetDefaultBackend, SetPeer/SetPeerWithToken,
RemovePeer, SetLibvirtURI, SetKubeContext, SetIncusRemote,
CTBackend's persist-on-detect) followed a Load → mutate → Save shape
with no lock across the sequence. Two concurrent mutations — two CLI
invocations, or two web requests hitting peer/context/folder routes at once
— could each load the same on-disk state and have one silently discard the
other's change on Save, with no error and no log line.

Adds pkg/config/store.go: a mutex-guarded cache of the default config
file's bytes, consulted by Load("") and Load(DefaultPath()). An explicit
path other than the default still bypasses the cache entirely, so existing
tests that read an arbitrary file (Load(configPath) with a temp file) keep
getting always-fresh reads exactly as before. Every Load("") still
unmarshals its own independent *Config — never a shared pointer — so a
caller mutating its result in place, which is what every setter does before
calling Save, can never corrupt another caller's copy.

A new mutate() helper holds the store's lock across a full
load-modify-save span; every existing setter is rewritten to go through it
instead of a bare Load+Save pair. This is what actually closes the
lost-update race — Save alone only makes the write itself atomic on disk,
it doesn't protect the read that preceded it.

No behavior change to any exported function's signature or return value.

Verification: go build ./..., go vet ./..., and gofmt -l all
clean. go test ./... passes for every package; pkg/web's
TestDemoMode_EndToEnd fails identically on an unmodified checkout in this
environment (missing OVMF/edk2 firmware packages, unrelated to this
change — confirmed by running the same test against a fresh clone of
main). Could not run -race here (no gcc/cgo available in this sandbox,
and just test/CI use -race -shuffle=on) — ran -shuffle=on -count=3
instead, which passed, and please confirm -race in CI.

Added TestMutate_ConcurrentWritesAllLand, which runs 20 concurrent
SetPeer calls and asserts all 20 survive. I verified this test actually
catches the regression it targets: temporarily reverting mutate() to the
old unlocked Load+Save pattern made it fail consistently across 5 runs,
losing 16-17 of 20 writes each time, before restoring the real
implementation. Also added TestLoad_ReturnsIndependentValues (guards the
shared-pointer hazard) and TestLoad_DefaultCacheInvalidatesOnHOMEChange
(guards the cache against serving stale bytes when DefaultPath() changes
mid-process, which the rest of the test suite relies on via a fresh HOME
per test).

Closes #359


Filed by architect agent (ACMM L6 — full mode)

— hive: agent=architect backend=pi model=kiro-api-key/claude-sonnet-5:high pi=0.87.1

pkg/config had no in-memory representation of the loaded config: every
read helper (Contexts, Peers, DefaultContext, DefaultBackend, HasBackend,
Folders, LibvirtURI, IncusRemote, ...) called Load("") independently, each
paying a fresh os.ReadFile + yaml.Unmarshal. pkg/web's request handlers
chain several of these per request (handleListVMs alone calls Peers() and
DefaultContext(), which itself calls Contexts()).

Every mutating helper (AddContext, RemoveContext, SetFolders,
SetDefaultContext, SetDefaultBackend, SetPeer/SetPeerWithToken, RemovePeer,
SetLibvirtURI, SetKubeContext, SetIncusRemote, CTBackend's persist-on-detect)
followed a Load-mutate-Save shape with no lock across the sequence, so two
concurrent mutations (two CLI invocations, or two web requests) could each
load the same on-disk state and have one silently discard the other's
change on Save.

Adds pkg/config/store.go: a mutex-guarded cache of the default config
file's bytes, consulted by Load() and Load(DefaultPath()) (an explicit
path still bypasses the cache, so tests reading an arbitrary file keep
getting always-fresh reads). Every Load() still unmarshals its own
independent *Config — never a shared pointer — so a caller mutating its
result in place (as every setter does) can't corrupt another caller's
copy. A new mutate() helper holds the store's lock across a full
load-modify-save span; every existing setter is rewritten to go through
it instead of a bare Load+Save pair, which is what actually closes the
lost-update race (Save alone only makes the write itself atomic on disk).

No behavior change to any existing function's signature or return value.
Verified with go build, go vet, gofmt, and go test ./... (all packages
pass; pkg/web's TestDemoMode_EndToEnd fails identically on an unmodified
checkout in this environment — missing OVMF/edk2 firmware packages, not
related to this change).

Added tests: TestMutate_ConcurrentWritesAllLand runs 20 concurrent SetPeer
calls and asserts all 20 survive — reverting mutate() to the old unlocked
Load+Save pattern makes this fail consistently (verified locally, losing
16-17 of 20 writes), confirming the test actually catches the regression
it targets. TestLoad_ReturnsIndependentValues guards the shared-pointer
hazard. TestLoad_DefaultCacheInvalidatesOnHOMEChange guards the cache
against serving stale bytes when DefaultPath() changes mid-process, which
the rest of the test suite relies on by setting a fresh HOME per test.

Signed-off-by: hanthor <hanthor@users.noreply.github.com>
@github-actions
github-actions Bot removed the request for review from hanthor September 25, 2026 05:21
@hanthor-hive-agent

Copy link
Copy Markdown
Contributor Author

Important

Held for human sign-off on the direction, not on the code.

This PR's only tracked rationale is #359, which the hive filed itself — issue #359 was filed by hanthor-hive-agent[bot] and no human has acknowledged it. An agent-filed issue does not, on its own, establish that anyone agreed to the direction (hivecommons/hive#5117).

The change may well be right; nothing here is a review of it. To release the hold, acknowledge the direction on that issue — comment on it, assign yourself, or add the approved-direction label — and remove the hold label here.

strategist[bot] and others added 2 commits September 25, 2026 07:44
Signed-off-by: strategist[bot] <strategist[bot]@users.noreply.github.com>
Signed-off-by: architect <architect@hive.kubestellar.io>
Signed-off-by: architect <architect@hive.kubestellar.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[architect] pkg/config re-reads and re-parses the config file on every call, with no lock across read-modify-write

1 participant