fix: state consistency across daemon, playlists, and wal-qt backend - #239
Merged
Conversation
time_of_day and day_of_week set nextChange inside their goroutine, so startPlaylist's synchronous read stored next_change_at=nil. That hid the UI progress bar and made missedEventChecker short-circuit forever.
Go timers use CLOCK_MONOTONIC, which does not advance across suspend, so timer playlists stalled after resume with a wall-clock NextChangeAt in the past and no watchdog to notice.
Set was a delete-then-insert with no unique index, so concurrent writers could leave two rows per monitor and a crash between the two statements lost the row entirely. BuildSnapshot now keeps the newest row per monitor.
Resume() spawned a second loop while the original stayed parked on a stopped timer, and the parked goroutine read s.timer without the mutex. Both schedulers now own one goroutine for life and park on resumeCh.
startPlaylist called Pause() after Start(), which raced the day_of_week immediate fire and applied a wallpaper for a paused playlist — colliding with the concurrent wallpaper.Restore on the fromPersisted path.
The daemon bus drops events for slow subscribers with no replay and the client backs off up to 60s, so everything published during a suspend was lost and the stores kept stale values indefinitely. Extracted the active-playlist refetch logic that already existed as a local closure inside useSetLastActivePlaylist into an exported refreshActivePlaylist(), now shared by both that hook's daemon-event listeners and the new useResyncOnReconnect hook.
Nothing in the stack knew a suspend had happened; the daemon caches its monitor list for the process lifetime and only refreshes on GET /monitors. IPCManager.notifySystemResumed() broadcasts go-daemon-event-system_resumed, which powerMonitor's "resume" listener in main.ts triggers; the renderer now treats system_resumed exactly like sse_reconnected. preload.ts's `on()` forwards go-daemon-event-* channels generically, keyed off the EventType union in electron/daemon-go-types.ts — added "system_resumed" there so the channel isn't rejected at the type level.
…ckend Shutdown -> SetActive -> Initialize was unguarded, so concurrent auto-mode ticks and manual sets could interleave and apply to a backend that had already been shut down while the DB recorded success.
The daemon supports a degraded no-backend start, so GET /wallpaper/current and the incompatible_backend error path on /wallpaper/set and /wallpaper/random turned a supported state into a panic and an opaque 500. Also switch SwitchActiveBackend's post-SetActive backend lookup back to Active() (from Get()) so it keeps working against MockRegistry fakes that only implement ActiveFn — Active() is guaranteed non-nil immediately after a successful SetActive under switchMu, and now that Active() returns nil instead of panicking the same nil-check covers both call sites.
Restore bypassed the per-backend apply gate, so a config PATCH concurrent with a playlist tick could reach the compositor out of order. Startup also raced Restore against RestorePersistedRuns, which assumes Restore already ran.
Task 7 made Registry.Active() return nil in the supported no-backend degraded mode, which turned these unguarded derefs into a panic on the web wallpaper config PATCH path. Also extracts missedEventTargetIndex so the suspend-recovery slide selection is covered behaviourally, not just by a wiring predicate.
wal-qt now reports per-target outcomes, so stop recording a wallpaper as applied on the strength of an ack that was sent before anything loaded.
Go carries out t.After(u) using the monotonic clock alone when both values hold a monotonic reading, and CLOCK_MONOTONIC does not advance while the machine is suspended. missedEventChecker compared two such values, so the watchdog whose entire purpose is catching a transition missed across a suspend was structurally blind to it: after an 8h sleep the check answered 'not due' until the original interval elapsed in awake time. Schedulers now publish wall-only deadlines (observable: no m=+ suffix) and the due-check strips monotonic readings defensively.
ViperManager used viper's built-in WatchConfig/OnConfigChange, whose watcher goroutine calls v.ReadInConfig() directly on the shared *viper.Viper, completely unguarded by ViperManager's mutex. Every config-writing method (mergeAndSet, persistKeyReplace, ResetToFactoryDefaults) rewrites the exact file being watched, so in production a config write's own fsnotify event races with concurrent reads/writes going through m.v — reproducible under `go test -race` and reachable via real HTTP config-write paths, not a test artifact. Replace viper's internal watcher with our own fsnotify loop (mirroring its directory-watch/symlink-swap algorithm) that takes m.mu before reloading m.v and releases it before invoking notifyCallbacks, so the watcher goroutine can never touch m.v concurrently with any other ViperManager method, and can't deadlock since it never fires from inside an already-locked call. Add a regression test driving concurrent SetBackendConfig/ UpdateConfig/GetConfig/GetSection, which fails under -race before this fix and passes after.
…ry code useSetLastActivePlaylist's lastSyncedPlaylistId was hoisted to module scope earlier today so refreshActivePlaylist (called from useResyncOnReconnect) could share it. Module-scoped mutable state only works because the hook happens to mount once app-wide, and it never resets between test cases. Move it into useActivePlaylistStore alongside the state it's already paired with; clear() now resets both together, preserving the original dedupe semantics exactly. Also remove isRetryableControlStatusErr and isRetryableUnixSocketDial from daemon/internal/backend/walqt/errors.go: both were left dead by the getStatusWithRetry removal in f1fe652 (per grep and `go run golang.org/x/tools/cmd/deadcode`), referenced only by their own now-deleted tests.
…rrectly The renderer previously inferred a slot's start time from when it first observed next_change_at, which is wrong whenever the UI connects mid-slot (GUI restart, daemon-then-renderer startup lag) or the deadline has already passed. The daemon now stamps SlotStartedAt (wall-clock only, alongside NextChangeAt) at every point it schedules a deadline, and the UI uses it to compute the real elapsed/total window, falling back to first-seen timing when talking to an older daemon. Overdue slots now render 100%/0 remaining instead of a dead "—" bar.
Remove the shadowtest package (self-testing only, no external callers), unused MockDB/MockMonitorProvider/MockScheduler test doubles, and unreferenced helpers ValidateAutoPriorities/categoryToMediaType, ContentToMediaType, and Controller.GetConfig. All confirmed dead via grep and go deadcode; unreleased/rewrite-friendly codebase per repo conventions.
The npm package "ci" has 0 imports anywhere in src/electron/shared/ globals/e2e and was misplaced under dependencies instead of devDependencies. Ran pnpm install to update the lockfile.
…ape hatch ViperManager's fsnotify watcher goroutine (introduced in 1e58de0) was never stopped, leaking one inotify instance per ViperManager for the life of the process. Add an idempotent Close() that stops the goroutine and releases the watcher, and call it from daemon shutdown via an io.Closer type-assertion (ConfigManager stays untouched so existing fakes/mocks keep compiling). Also audited every Viper() caller: two (image.Processor's stored-but-unused configViper field, walqt's monitor provider) retain the raw *viper.Viper beyond startup and read it later, unguarded by ViperManager's mutex — a real latent race with the watcher goroutine, not just ordering luck. Document the constraint precisely on Viper() rather than deleting it, since those callers genuinely need a long-lived instance and live outside this package's scope.
The spec declared BackendCapabilities.media_types, but backend.Capabilities has always serialized as content_kinds (electron/daemon-go-types.ts and the test mock already agreed on that) — the spec was the thing out of sync. ci:types only diffs the generated TS against the spec, never the spec against the Go struct, so this could drift forever without failing CI. Add a Go test that reflects over backend.Capabilities' json tags and checks each appears in the BackendCapabilities schema, closing that hole.
cfg.AllowNetworkWallpapers was parsed, defaulted, and exposed in the UI, but controlClient.setAllowNetworkWallpapers was never called from anywhere, so the setting had no effect (wal-qt always ran with its own fail-closed default). Push it from Apply, gated on the load being a web wallpaper, right before /wallpaper/load — this covers daemon startup with an already-running wal-qt (Apply runs as part of restore) and a runtime PATCH to the backend config (UpdateBackendConfig re-applies the active snapshot), without adding a round-trip to every image/video Apply.
setImagePresentation, the topology-matching helpers (TopologyMonitorMatch, TopologyMonitorContainingCenter, TopologyMonitorMatchByPosition, ResolveParallaxMonitor, approxEqTopology), and the NewWalQtWithHTTPClient test helper had no callers anywhere in the daemon. Removes their tests too (mapping_test.go existed solely to exercise the topology helpers).
…backends All six backends (awww, feh, hyprpaper, mpvpaper, swaybg, wal-qt) and wal-qt's monitor provider captured the *viper.Viper handed to RegisterDefaults and read it later at runtime — every Apply, every GET /monitors — unsynchronized. ViperManager's config-file watcher goroutine reloads that same instance (ReadInConfig, a map write) under its own mutex, which those retained-pointer reads never took: a proven, reachable data race on the daemon's hottest path (confirmed via -race, 206 warnings from the reproduction in this commit's regression test run against the old code). Add backend.ConfigReader, a 6-method interface covering exactly what the backends call (GetString/GetInt/GetFloat64/GetBool/GetStringSlice/IsSet), and backend.ConfigReaderReceiver, an optional interface backends implement via SetConfigReader. *config.ViperManager implements ConfigReader with its own RWMutex-guarded methods. main.go now calls SetConfigReader(cfg) right after RegisterDefaults for every backend, and passes cfg (not the raw viper) into walqt.NewMonitorProvider. Backend struct fields change type from *viper.Viper to backend.ConfigReader; every internal read call site (w.v.GetString(...) etc) is untouched. Also drop image.Processor's configViper field/param: grep confirmed it was never read, so it was dead retained state of the same shape. internal/backend/walqt/config_race_test.go is the permanent -race guard, covering both WalQt.loadConfigFromViper and the monitor provider's controlConfig against a real ViperManager under concurrent SetActiveBackendType calls (which exercise the same reload path as the file watcher).
daemon/docs/swagger.yaml was Swagger 2.0 generated by swag from @router annotations, last regenerated May 22, and fed nothing downstream (swag isn't even installed, so ci:openapi couldn't run). daemon/docs/openapi.yaml is OpenAPI 3.0.3, hand-maintained, and feeds generate:types -> electron/daemon-go-types.generated.ts. Consolidating to a single spec per maintainer decision. Per-handler swag annotation comments (@summary, @router, etc.) are left in place; their fate is a separate decision owned by other in-flight work.
ci:check silently skipped the spec-drift check (ci:types) and the race detector (test:daemon:race), so both existed only as one-off manual commands that rot the moment someone forgets to run them. test:daemon:race replaces test:daemon:unit rather than running alongside it: verified both scripts execute the exact same set of 610 tests (diff of `go test -v` output is empty), so running unit mode and then race mode back to back would just repeat identical work for no extra coverage. Race mode is strictly stronger (same tests plus data-race detection) at a modest cost (~7.5s vs ~2.5s locally), so it's a straight upgrade in the same slot. ci:types is placed right after build:daemon, since build:daemon already regenerates the types as a side effect of make daemon; the extra step is just the git diff check to catch an uncommitted/stale generated file.
Committed unformatted earlier today; now blocks ci:check since format:check became part of the ratchet.
…yped structs Gives all 25 remaining untyped daemon handler responses a named Go struct (one per file, matching the existing WallpaperCurrentResponse convention), so a follow-up task can assert Go response shapes against openapi.yaml. Wire format is unchanged; added marshal-comparison tests for endpoints with no existing response-shape test coverage.
swagger.yaml and ci:openapi were removed earlier; the //go:generate swag directive, its package-level @title/@version/@basepath annotations, and the generate:openapi script that triggered it were the last remnants. swag was never installed, so none of it could run. package.json key order is oxfmt's canonical output — format:check is part of ci:check now, and the file did not conform.
Viper() carried 23 lines of prose for a one-line getter, and it had already gone stale: it warned that walqt's monitor provider retains a raw *viper.Viper, which stopped being true when ConfigReader landed. Kept only what the code cannot state itself — chiefly that Round(0) is load bearing, since Go compares monotonic times without the wall clock.
Drops an IDAllocator usage example that duplicated a real call site ten lines away, an SSE frame-format block visible in the writer, and an xrandr sample already shown next to each parser. Kept the wl_output limitations and the source-target contract: neither is derivable from the code, and both are silent-failure traps.
…rections Generalises the walqt-only BackendCapabilities check into one test package (internal/apispec) covering every schema in openapi.yaml that has a corresponding Go type. For each mapped schema, asserts json tags on the Go struct and spec properties are the same set: - Go -> spec: every json tag must appear in the schema (catches an undocumented/renamed Go field, e.g. the historical media_types vs content_kinds drift). - spec -> Go: every schema property must exist on the Go struct (catches a spec property documenting behaviour Go never implemented). Replaces the single-purpose capabilities_spec_test.go in internal/backend/walqt, which this supersedes.
swag was never installed and swagger.yaml/ci:openapi/go:generate swag init were already deleted; daemon/docs/openapi.yaml is now the sole API spec source of truth. The // @Summary/@Tags/@Param/@Success/@Failure/@router annotation blocks generated nothing and could silently drift from it, so strip them, keeping the plain doc-comment lines (including the tie-break prose on WallpaperHandler.GetCurrent).
… test BackendCapabilities documented transitions/per_monitor/daemon_process, none of which exist on backend.Capabilities (dead fields, likely stale since before the struct was last trimmed) — removed. BackendInfo was missing the `active` field backend.BackendInfo actually serializes. APIError was missing error_code and meta, both populated by httpjson.WriteStructuredError. Also replaces GenericJSON/inline-object placeholders with dedicated schemas for the 25 handler responses that gained named Go structs in 41ba414, plus the pre-existing WallpaperCurrentResponse, and adds the previously undocumented GET /capabilities path. HealthzResponse in particular was pointed at StatusOk, which only declares `status` — Go actually also sends monitor_stack_version and monitor_provider_order. Regenerated electron/daemon-go-types.generated.ts to match.
…its interval The test assigned ts.interval while runLoop was already parked on a timer built from the old 3600s value, so a tick only arrived if the reconcile's own channel sync happened to land after the override — 5 failures in 12 runs. AfterManualNavigation blocks until the loop restarts its wait. Matters more now that test:daemon:race gates ci:check.
# Conflicts: # daemon/internal/backend/walqt/walqt.go # daemon/internal/wallpaper/restore_test.go
Watch logind's PrepareForSleep signal and, on resume, force a monitor re-detect and re-run Restore() so a monitor dropped across a suspend is not left wallpaper-less. Best-effort: no logind/system bus logs a warning and runs on.
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.
Summary
A batch of state-consistency fixes accumulated across the daemon, playlist manager, and wal-qt backend integration, plus follow-on cleanup that fell out of them:
Active()returns nil instead of panicking when no backend is active, guarding web config push against a nil active backend, orderingRestorebefore playlist restore, treating wal-qt load as successful only when targets confirm, actually transmittingallow_network_wallpapersto wal-qt, and no longer retaining raw*viper.Viperacross the six wallpaper backends.Close.daemon/docs/openapi.yamlis now the single source of truth for the HTTP API — removedswagger.yaml, theci:openapiscript,//go:generate swag init, and the now-orphaned// @Summary/@Tags/@Param/...swag annotation comments across alldaemon/internal/handler/**files (doc-comment prose, e.g. onWallpaperHandler.GetCurrent, was preserved). Addedinternal/apispectests that verify openapi.yaml schemas against the corresponding Go structs in both directions, and fixed drift the new test caught (staleBackendCapabilitiesfields, missingactiveonBackendInfo, missingerror_code/metaonAPIError,HealthzResponsepointed at the wrong schema).map[string]anyHTTP responses with typed structs, trimmed comments that only restated the code.Test plan
go build ./...(daemon)go vet ./...(daemon)gofmt -l ./internal/ ./cmd/cleango test -race ./... -count=1(daemon) — all packages passpnpm run ci:check(repo-wide) — not run in this session;electron/daemonForward.tshas an uncommitted maintainer edit outside this branch's scope that failsformat:check, so individual daemon-side commands were used instead