Skip to content

feat(devices): TC001 as an awtrix-ng registry device with a config facade - #360

Merged
tarakanof merged 8 commits into
mainfrom
feat/230-clock-device
Oct 9, 2026
Merged

tarakanof merged 8 commits into
mainfrom
feat/230-clock-device

Conversation

@tarakanof

Copy link
Copy Markdown
Owner

Closes #230

Phase 2 of per-device apps (spec Specs/ember/2026-10-04-per-device-apps-design.md). Server only; the app binding is #359.

What changed

Clock record (clock_device.go)

  • A good probe of the effective clock URL creates the record. A good probe is GET /api/v1/device returning boardType: awtrixng plus a uid, through the shared clock-health probe. A discovery swap also creates it.
  • Record fields:
    • id clock-<last6 of uid> (full uid on a collision); hw_id = uid
    • name Clock <SUFFIX>; no token and no stored config
    • creation bumps the epoch
  • Later probes set last_seen{seen_at,fw,ip,rssi,uptime_s} and persist on the checkin rule (10 min).
  • URL tiers stay in clock_url.go.

Kind gates

  • Device auth skips non-knob records.
  • rotate and registry checkin return 400 for awtrix-ng.
  • stats, coredumps and ota by id return 404 (findKnob).
  • The OTA loops skip the clock kind.
  • POST /v1/devices still mints knobs and clients only.
  • PATCH renames the record, and a probe never resets the name.
  • DELETE returns 204, and the next good probe re-creates the record with its default name. This is how a swapped-out clock's record goes away.
  • Doctor judges the clock by last_seen: warns past 5 min, says clock disabled under EMBER_CLOCK=off.

Façade GET/PUT /v1/devices/{clock}/config (clock_config.go)

  • apps.agents: usage_cards, usage_per_model (usage_json); hidden_tools (display_hidden_apps)
  • apps.focus: focus_color, break_color (settings_json)
  • apps.weather (weather_json):
    • on (rotate_in_apps), native_icon, forecast, forecast_hours, air, moon, overlay
    • popups.{on_change, sun, severe, native_icons, interval_minutes, duration_seconds}, icon_ids
  • apps.calendar: on (meetings enabled), tile_lead_minutes, popup_lead_minutes (meetings_json)
  • rotation.{order, disabled}: the clock's own app list (NG /api/v1/apps)
  • Excluded on purpose:
    • source fields
    • sounds
    • the agent-card spine flags (stay per Mac in producer.env)
    • rotation dwell (only in config.json)
    • The knob reads none of these.
  • The full table is in ARCHITECTURE "Clock record and config façade".

PUT

  • Merge rules are the knob's:
    • objects merge per field
    • arrays and icon_ids replace whole
    • an unknown key or app is 400
    • {} writes nothing
  • Each slice goes through its own settingSpec (setting.stage, the same apply and validation as the old endpoint):
    • validated first on a copy
    • then applied, persisted and hidden tools replaced in one tryUpdateConfig under cfgMu
    • after hooks run after
  • rotation is sent to the clock (the PUT /v1/device/apps body) after validation and before the slices. A failed clock write returns 502 and changes nothing.

Version and epoch

  • config_version is 31 bits of the SHA-256 of the composed config, stored on the record.
  • It is resynced after every config change, hidden-tool change, record creation and app-list read. The epoch moves once per change, through any endpoint.
  • The app list is cached in memory. PUT /v1/device/apps forgets it, so that route makes no extra clock request.
  • Boot and /admin/reload pause the resync while reapplying, so the epoch moves at most once.

Compatibility

  • Old endpoints are unchanged. The one exception is GET /v1/device/apps, which also caches the list it proxies.
  • The coordinator reads the same fields.
  • Knob goldens are byte-identical: testdata/devices is unchanged.
  • Clock goldens live in testdata/clock/, so cinder's fixture sync doesn't pick them up.
  • A clock change bumps the shared epoch, so the knob makes one extra checkin. The spec accepts this, and DEVICE-PROTOCOL now notes it.

Evidence

  • gofmt clean, go vet ./... clean, and go test ./... -race -shuffle=on passes.
  • New tests:
    • golden composed config, default and custom
    • façade PUT vs old-endpoint PUTs give identical KV blobs, effective config, clock writes and composed config
    • merge rules; 16 bodies that must return 400 and change nothing; the {} no-op
    • a rotation write failure changes nothing
    • record creation from an httptest awtrix-ng stub, a non-NG board, a discovery swap, an id collision
    • kind gates: rotate, checkin, auth (even with a planted hash), stats, ota, coredumps, mint, the OTA loops
    • delete then re-create; rename survives a probe
    • doctor lists the clock (ok, stale and disabled)
    • epoch bumps through old endpoints, not on source-only changes, once per façade PUT, at most once on reapply
  • Smoke run: scratch port, temp DB, EMBER_MDNS_ADVERTISE=0, local Python stub with auto_rediscover:false. The real clock, server and knob were not contacted.
    • GET /v1/devices → {"id":"clock-112233","kind":"awtrix-ng","hw_id":"aabbcc112233","name":"Clock 112233","config_version":120194409,...,"last_checkin":null,"last_seen":{"fw":"1.0.13","ip":"127.0.0.1","rssi":-55,"uptime_s":100}}
    • GET /v1/devices/clock-112233/config → 200 with X-Ember-Config-Version
    • a façade PUT with weather and rotation → 200, and the stub's app order changed
    • POST …/rotate → 400 rotate applies to kind "cinder-knob" only
    • doctor devices → registered=1 clock-112233 seen=6s ago
    • The smoke run showed a façade PUT with rotation bumping the epoch twice. Fixed, with a test.

Left for #232

  • Store presentation on the record, with a boot migration.
  • Old endpoints become aliases.
  • Sounds under Clock.
  • Whether agent-card flags move server-side.

…g facade

Phase 2 of per-device apps (#230): both displays are now addressed as
/v1/devices/{id}, and the clock's presentation settings read and write
as one config without moving any storage yet (#232 does that).

The clock record (id clock-<last6 of NG uid>, no token) is created by
the first good probe of the effective clock URL or a discovery swap and
keyed by the uid GET /api/v1/device already returns; later probes set
last_seen on the checkin persistence rule. URL tiers stay in
clock_url.go. Device auth, rotate, checkin and the OTA loops skip the
kind; DELETE drops the record until the next good probe re-creates it,
which is how an old clock's record goes away after a swap. Doctor
judges the clock by last_seen instead of a checkin.

GET/PUT /v1/devices/{clock}/config composes apps.{agents, focus,
weather, calendar} and rotation from usage_json, settings_json colours,
weather_json, meetings_json, display_hidden_apps and the clock's own
app list, and splits a PUT back through each slice's settingSpec in one
tryUpdateConfig, so validation and persistence match the old endpoints
byte for byte. Same merge rules as the knob. The rotation patch goes to
the clock first, after validation, so a failed clock write changes
nothing.

config_version is a hash of the composed config, stored on the record
and resynced after every config, hidden-tool or app-list change, so the
devices epoch moves whichever endpoint made the change. Boot and reload
reapply with the resync paused to move it at most once.

Old endpoints, the coordinator and the knob goldens are unchanged;
PUT /v1/device/apps makes no extra clock request (it forgets the cached
app list instead of re-reading it).

Spec: Specs/ember/2026-10-04-per-device-apps-design.md
ARCHITECTURE gets the record lifecycle, the facade field table, merge and version rules; API, openapi, RUNBOOK and DEVICE-PROTOCOL note the new kind, the 400 on rotate and that a clock change moves the shared epoch.
A LAN host answering GET /api/v1/device with a fresh uid on every probe
appended a persisted awtrix-ng record each time, growing devices_json
without bound and bumping the epoch, which woke every knob.

Phase 2 has one clock, so a different uid at the effective clock URL
now rewrites the existing record in place: new id and hw_id, last_seen
reset, created_at and a user-set name kept, one epoch bump. Records are
only created from a probe of effectiveClockURL() or the discovery
candidate a swap just made effective; the probe body is already read
through the clock client's 1 MiB cap. The uid pattern is tightened to
32 characters.
@tarakanof

Copy link
Copy Markdown
Owner Author

Codex review

No deadlock was found between the registry mutex and cfgMu, and no kind-gate bypass.

Blocker

  • clock_config.go:342: persistence isn't atomic. Each slice is a separate SQLite write, and persist only logs failures.
    • Example: the rotation and usage_json writes succeed, then weather_json fails. The PUT still returns 200 and publishes the full in-memory config. After a restart, the stored config is only partly applied.

Should-fix

  • clock_config.go:315: the merge happens before cfgMu is taken, and the whole next.Apps is applied under the lock later.
    • Example: PUT A waits on the rotation HTTP call while PUT B changes usage_cards. A then writes back its old usage_cards, a field A never sent.
  • clock_health_http.go:334: probe results are registered without checking that the probed URL is still the effective one.
    • Example: a probe of clock A finishes after rediscovery has switched to B and registered B. A replaces B, then the next probe of B replaces A. The record flips identity and the epoch churns, waking knobs.
  • clock_config.go:330: a successful rotation write doesn't invalidate the cache before the readback.
    • Example: the readback GET times out. The handler returns 200 with the old rotation and the old hash.
  • clock_device.go:189: computing the hash and storing it aren't serialized together.
    • Example: goroutine 1 computes H1, goroutine 2 computes and stores H2, then goroutine 1 stores H1. The stale value stays, and the epoch moves an extra time.
  • clock_config.go:128: composing the config doesn't read a consistent snapshot.
    • Example: a GET reads the old Config, a façade PUT replaces the hidden tools, then the GET reads the new hidden tools. The body and hash describe a state that was never committed.

Test gaps: partial persist failure, concurrent PUTs, a stale probe after rediscovery, a failed rotation readback, and the hash/store race.

@tarakanof

Copy link
Copy Markdown
Owner Author

Opus review

Independent read-only review of 0aebdc4 (3 commits). go test -race -shuffle=on ./... passes in a scratch copy. No blockers. One should-fix is a reproduced lost update.

Should-fix

1. A façade PUT reverts concurrent slice writes (lost update). cmd/ember/clock_config.go:315-349
cur is composed outside cfgMu (L315), merged into next, and the clock round trip runs (L327-330). Then inside tryUpdateConfig every slice is re-staged from next.Apps, which holds full values for all four slices taken from the stale cur (L339). stage writes any field that differs, so any slice edited in that window is reverted, even one the patch never named.
Scenario (reproduced with a scratch test that holds the clock stub's mutex during PUT /api/v1/apps/order): the façade sends PUT {"apps":{"calendar":{"tile_lead_minutes":15}},"rotation":{…}}, and while it waits on the clock, PUT /v1/weather/config {"moon_phase":false} lands. When the façade finishes, moon_phase is back to true in memory and in weather_json. The window is as long as a clock write, which takes seconds on the lossy clock link. Two Macs, or the app plus a script, can hit it.
The knob does not have this problem: putConfig merges inside the registry lock. The spec's "same merge rules as the knob" and "atomic across slices" claims depend on it.
Fix: keep the dry run before the clock write for validation. Inside the tryUpdateConfig closure, compose from c, using the hidden tools read there, then merge the patch again and stage. A test like the one above would pin it.

2. config_version sync is check-then-set, so the stored version can go backwards. cmd/ember/clock_device.go:185-197
syncClockConfigVersion computes the hash without a lock, then setClockConfigVersion stores it under r.mu. If two triggers run at once, A can compute v_old, B can compute and store v_new, and A then stores v_old.
Scenario: the app saves weather (tryUpdateConfig → sync) at the same moment it reads GET /v1/device/apps (noteClockApps → sync). The record's config_version in /v1/devices ends up as a hash of a config that no longer exists, while the GET/PUT header shows the current hash. The epoch also moves twice. Nothing corrects it until the next trigger.
Fix: serialise compute+store with a mutex in clockSyncGate, or compute inside setClockConfigVersion under r.mu. The compose only takes appsMu and an atomic load, so this adds no ordering problem.

3. A uid that changes on every probe still bumps the epoch every probe. cmd/ember/clock_device.go:109-124
0aebdc4 stops devices_json from growing, but its own commit message gives "bumping the epoch, which woke every knob" as the problem. The rewrite path still runs mutateLocked (a SQLite write) and st.Epoch++ on every probe whose uid differs. A host at the effective clock URL that answers with a fresh uid each time therefore gets an epoch bump and a blob write about every 30 s (clockProbeTTL). Every knob checks in about twice as often, and the record id flips each time, so an app holding the old clock-… id gets 404s.
Fix (pick one): only rewrite after the same new uid has been seen on two probes in a row; rate-limit identity rewrites, for example to one per 10 min; or skip the epoch bump on an identity-only rewrite, since the knob reads nothing from the clock record and the composed config, the only thing the spec ties to the epoch, did not change. Then fix the commit/docs claim.

Nits

4. rotation means different things in GET and PUT. clock_config.go:265-278 vs :224/:325-327
GET's order lists every app, disabled ones included. PUT's order follows NG, where it names the enabled apps. If a client PUTs back the rotation it got from GET, the request lists hum in both order and disabled, and what happens depends on NG. A PUT that sends back an unchanged rotation also always writes to the clock, so it returns 502 when the clock is offline even if only server-side slices changed. Two options: make GET's order the enabled apps only (with disabled listed separately), or skip the clock write when next.Rotation equals the cached rotation. The doc line "Its keys follow NG, not the merge" explains the current behaviour but does not remove the trap.

5. The PUT is atomic in memory only. clock_config.go:342-347, docs/openapi.yaml ("one atomic update")
s.persist and persistHiddenAppsLocked are separate PutSetting calls that log failures and do not return them. If one row fails, the response is 200 with every slice committed in memory, but after a restart only some of them come back. In the case you asked about (the clock write succeeds, then the slice commit fails), the commit can only fail by re-validating against a c changed concurrently, which is essentially unreachable. If it does happen, the 400 invalid settings body hides the fact that the clock was already reordered. Suggest the docs say "atomic in memory; per-slice persistence as the old endpoints".

6. Reordering through the old endpoint moves the epoch twice. device_display.go:89 (forgetClockRotation)
PUT /v1/device/apps sets the rotation to nil (one bump), and the app's follow-up read sets it again (a second bump). That costs two pointless knob checkins per reorder. Also, the rotation includes origin: pushed Ember tiles that come and go, so most GET /v1/device/apps reads change the hash and bump the epoch even when no setting changed. That matches the spec's "keep the rule simple", but it is worth one sentence in ARCHITECTURE "Version". refreshClockRotation after the PUT, instead of forgetting the rotation, would give one bump.

7. hidden_tools limits are stricter than the old endpoint. clock_config.go:206-219 vs apps_http.go:139-153
/v1/apps accepts any non-empty name with no count cap. The façade rejects names over 64 runes and lists over 64 names. If a name or list over those limits is set through the old route, the façade GET returns it and the next PUT that sends it back gets a 400.

8. A probe and a rediscover swap can race. clock_health_http.go:266,330-335 and device.go:78
The probe snapshots base but dials whatever effectiveClockURL() is at call time, and it records the uid with no check that the URL is still the effective one. With two NG boards on the LAN and the lossy link, rediscover can swap to B while a probe of A is still in flight. That probe completes and rewrites the record back to A (id flip, epoch bump), and the next probe flips it to B again. This happens once per swap, so it is low impact. Passing base through, or checking cfg.effectiveClockURL() == base before observeClock, closes it.

Checked, OK

  • Kind gates: authenticate skips non-knob records, and TokenSHA256 is empty anyway. rotate and checkin return 400. Stats, coredumps (diagnostics → findKnob), OTA get/put and otaTargets/Keeps/Retire are knob-only, so a clock id gets 404. provision only matches knobs, and the knob-/clock- prefixes cannot collide. self/* uses the authenticated id. The doctor handles EMBER_CLOCK=off.
  • Lock order: tryUpdateConfig now unlocks cfgMu before syncClockConfigVersion takes r.mu. replaceHiddenApps (appsMu) runs under cfgMu, and no path takes cfgMu while holding appsMu or r.mu. onChange → notify under r.mu was already there. I found no deadlock.
  • Hash: json.Marshal sorts map keys, hidden_tools is sorted, and the rotation keeps NG order. The pause and deferred sync give one epoch bump per façade PUT (tested). On boot and reload the epoch moves at most once, as documented.
  • Façade vs old endpoints: TestClockConfigFacadeMatchesOldEndpoints checks that store blobs, effective config, clock writes and the composed GET are all identical.
  • Merge: objects merge, arrays replace, icon_ids replaces (null clears it), unknown keys and schema≠1 return 400, and {} does nothing (tested). The weather slice's omitempty on icon_ids means an empty map vs nil does not cause a write.
  • view() clones before truncating seen_at, so the read is race-free.
  • Knob: the persisted knob JSON is unchanged (omitzero never triggers for a filled knobSettings), and no knob golden changed. DEVICE-PROTOCOL documents the extra checkin that a clock change costs.
  • No code comments added. Docs match the code, except deploy: Unraid host networking + writable store volume for discovery; release-driven redeploy docs #3 (the commit and docs say the epoch churn is fixed) and chore: pre-push hook mirroring CI #5 ("atomic").

PutSettings upserts a set of settings rows in one SQLite transaction and rolls back on any failure, so a multi-slice settings change is stored whole or not at all. The clock config facade needs it (#360 review).
Review findings on #360 (Codex and Opus):

- Persistence: every changed slice and the hidden tools are stored in
  one SQLite transaction before the new config goes live; a failed
  store is a 500 and leaves memory and the store unchanged.
- Lost update: the patch is merged again inside tryUpdateConfig onto
  the config and hidden tools of that moment, so a write that landed
  while the clock call ran is kept. /v1/apps toggles take cfgMu too.
- Version: compute and store of config_version are serialised, and the
  composed config (GET body and hash) comes from one snapshot.
- Identity: a probe counts only if its URL is still the effective one;
  a new uid replaces the record only after two sightings in a row, so a
  flapping or fresh uid never flips the record or moves the epoch.
- Rotation: GET's order now means what PUT's does (enabled apps;
  disabled listed apart), pushed Ember tiles are left out of it and of
  the hash, an unchanged rotation is not written to the clock, the
  cache is dropped after a write and a failed readback gives null.
  PUT /v1/device/apps no longer bumps the epoch twice: the version
  moves once, at the next read of the list.
- hidden_tools takes what /v1/apps takes, so a GET body can be PUT back.
ARCHITECTURE and openapi described the facade PUT as atomic while each slice was a separate write, and the uid rewrite as ending epoch churn; they now match the code after the #360 review fixes.
@tarakanof

Copy link
Copy Markdown
Owner Author

Review replies (fixed in fb3cbc7, 13020cb, 1f49188)

Codex

  • Blocker, non-atomic persistence: fixed.
    • New Store.PutSettings writes every changed slice plus display_hidden_apps in one SQLite transaction.
    • The config and hidden tools go live only after that commits. A failed store returns 500 and leaves both memory and the store unchanged.
    • If the clock order was already written, the error body and the log say so.
    • Tests: TestPutSettingsRollsBackOnFailure, and TestClockConfigPersistFailureChangesNothing, which uses a SQLite trigger to fail weather_json partway through the batch.
  • Merge outside cfgMu (lost update): fixed.
    • The dry run still runs before the clock write.
    • Inside the tryUpdateConfig closure the patch is merged again onto that moment's c and hidden tools, then staged.
    • /v1/apps toggles now take cfgMu.
    • Tests: TestClockConfigPutKeepsConcurrentOldEndpointWrite and TestClockConfigConcurrentFacadePutsKeepBoth. Both hold the stub's order PUT open while the other write lands.
  • Stale probe: fixed.
    • A probe result counts only if the URL it probed is still effectiveClockURL() when the result arrives.
    • Test: TestStaleProbeAfterRediscoveryNotRegistered.
  • Rotation readback: fixed.
    • After a successful write the cache is dropped and the list read again.
    • If that read fails, the reply's rotation is null and the version is computed from that body. The old list is never returned.
    • Test: TestClockConfigRotationReadbackFailureReturnsNull.
  • Hash compute/store race: fixed.
    • clockSyncGate.mu serialises compute and store.
    • Test: TestClockConfigVersionSyncRace, 40 concurrent triggers, then the stored version must equal the current hash.
  • Inconsistent snapshot: fixed.
    • composeClockConfig reads the config and hidden tools under cfgMu. Hidden-tool writers hold cfgMu too.
    • The GET body and its hash come from that one snapshot.

Opus

  1. Lost update: fixed. Same change as Codex's lost-update finding above.
  2. Version going backwards: fixed. Same change as Codex's hash race finding above.
  3. uid churn: fixed. A new uid replaces the record only after the same uid is seen twice in a row. Fresh or alternating uids never flip the record or move the epoch, and a last_seen update never bumps it. Tests: TestFlappingClockUIDNeitherFlipsNorBumps, TestLastSeenUpdateLeavesEpochAlone, TestClockUIDReplacedAfterTwoProbes. The docs no longer claim more than this.
  4. rotation meaning: fixed.
    • GET's order now lists the enabled apps and disabled lists the rest, the same as PUT and NG, so a GET body can be sent back.
    • A PUT whose rotation equals the last list read from the clock does not write to the clock, so no 502 while the clock is offline.
    • Test: TestClockConfigRotationRoundTripsWithoutClockWrite.
  5. Atomic in memory only: fixed. The store write is now transactional (see the Codex blocker reply). The openapi and ARCHITECTURE wording is accurate now.
  6. Double bump on reorder, and pushed tiles: fixed.
    • PUT /v1/device/apps leaves the cache alone, so the version moves once, at the next read of the list.
    • Pushed Ember tiles are left out of rotation and the hash.
    • Tests: TestClockConfigOldAppOrderMovesVersionOnceAtNextRead, TestClockConfigPushedAppsDoNotMoveVersion.
  7. hidden_tools limits: fixed.
    • The limits now match /v1/apps: any non-empty name, no cap.
    • Test: TestClockConfigHiddenToolsFromOldEndpointRoundTrip (70 names of 100+ characters).
  8. Probe/swap race: fixed. Same change as Codex's stale-probe finding above.

All the new tests fail against 0aebdc4 except the race test, which is probabilistic. go test ./... -race -shuffle=on passes.

@tarakanof

Copy link
Copy Markdown
Owner Author

Re-check

Read-only re-check of fb3cbc7, 13020cb and 1f49188 (git diff 0aebdc4..HEAD). go vet is clean, and go test -race -shuffle=on ./... passed 3 out of 3 runs in a scratch copy. The new clock tests also passed 30 out of 30 with -race -count=30.

should-fix

An unchanged-rotation skip based on a stale cache drops a real reorder and still returns 200. PUT /v1/device/apps no longer clears clockRotation, and putClockConfig compares the patch against cur.Rotation, the cached list, without reading the clock again. I reproduced it in a scratch test:

  1. GET the façade. The cache now holds time,date.
  2. PUT /v1/device/apps {"order":["date","time"]}. The clock now holds date,time and the cache still holds time,date.
  3. Façade PUT {"rotation":{"order":["time","date"],"disabled":["hum"]}} returns 200 with rotation: time,date, but there are 0 order writes and the clock still starts with date.

The same thing happens after any change the server hasn't read: a reorder in the clock's web UI, or an app re-read that failed after the old-route PUT (this is likely given the known ~44 % loss on the clock link). The client is told the clock matches when it does not.

Possible fixes, either one:

  • In handleDeviceAppsPut, call a.clockRotation.Store(nil) without a sync. The version still moves once, at the next read, because the stored version is only recomputed on a sync.
  • Skip the write only when the list was read during this request. Do a refreshClockRotation before comparing; if that read fails, write anyway, which keeps the old 502 behaviour.

The docs paragraph ("equals the last list read") describes the code accurately but hides this consequence. Please add a test that runs these three steps.

nit

  • TestClockConfigVersionSyncRace never failed on 0aebdc4: 0 out of 40 runs under -race. The reply calls it probabilistic, but as written it doesn't guard the clockSyncGate.mu fix. A hook that forces the compute→store interleaving would make it bite.
  • TestLastSeenUpdateLeavesEpochAlone passes on 0aebdc4. That's fine for a guard test, but the reply says "all the new tests fail against 0aebdc4 except the race test". The other 11 new cmd/ember tests, and the regenerated TestClockConfigGolden, do fail on 0aebdc4, as claimed (the moved test files compile there). The PutSettings tests can't compile on 0aebdc4.
  • openapi 500: "The settings store failed; nothing changed." The order may already have been written. The endpoint description says so; the response text could say "no settings changed".

Verified, clean

  • Store.PutSettings: it runs on one sql.Tx with SetMaxOpenConns(1), so the tx holds the only connection and other writers wait. Every Exec goes through tx. The deferred Rollback on the named err covers both exec and commit failures. Memory changes only after the commit: putSettingsBatch → setHiddenAppsMemory → return nil → cfg.Store, and an error leaves cfg and hiddenApps untouched. The trigger-based tests fail on 0aebdc4 and pass on HEAD.
  • Lock order: the order is always clockSync.mu → cfgMu → appsMu, then the registry mu after cfgMu is released, then changes.mu. Nothing takes cfgMu or clockSync.mu while it holds appsMu or the registry mu. onChange only touches changes.mu. syncClockConfigVersion is never reached while cfgMu is held: tryUpdateConfig and setAppHidden call it after unlocking, and observeClock computes the version before seenClock takes the registry lock. I found no deadlock path.
  • No clock HTTP under cfgMu: the order PUT and the readback run before tryUpdateConfig. The work done under cfgMu is the merge, staging and SQLite only.
  • Snapshot: composeClockConfig reads cfg and the hidden tools together under cfgMu, and the body and the hash come from the same struct. rotation is a separate atomic, but body and hash still agree.
  • Re-merge: TestClockConfigPutKeepsConcurrentOldEndpointWrite and TestClockConfigConcurrentFacadePutsKeepBoth hold the order PUT open, then check that both writes survive. Both fail on 0aebdc4.
  • Identity: with no record, the first probe or discovery creates one (d == nil skips the pending gate). A swap converges on the second sighting (two probes, or discovery then a probe). Alternating uids reset pendingClockUID and never flip the record. Dropping the collision check in clockDeviceID is safe with a single clock record.
  • Stale probe: effectiveClockURL() == base compares two values from the same function, so normalisation can't make them differ. Any remaining TOCTOU is covered by the two-sighting rule.
  • Rotation readback: the cache is cleared before the read, a failed read returns null, and the version header is the hash of that body. A GET body with rotation: null sent back produces no clock write. No client consumes the façade yet: the macOS app uses /v1/device/apps, and cinder never reads clock config.
  • Goldens: only testdata/clock/config_{default,custom}.json changed. The ember (pushed) entry left order, and hum moved to disabled only, which matches the stub. No knob fixture changed, and TestKnobConfigUnaffectedByClockRecord passes.
  • hidden_tools: the request size is bounded by decodeJSON's 1 MiB MaxBytesReader on the owner/admin route. Names are now as unbounded as /v1/apps.
  • Docs: ARCHITECTURE and openapi match the code, apart from the openapi 500 nit above.
  • No code comments were added, and gofmt is clean.

… write

Re-check on #360: the unchanged-rotation skip compared the patch with
the cached list, so after a reorder the server hadn't read (old route,
the clock's web UI, a failed read) a resent rotation returned 200
without reaching the clock. The facade now reads the list during the
request and skips only when that read succeeds and matches; a failed
read writes. PUT /v1/device/apps clears the cache again, without a
resync, so the version still moves once, at the next read.

TestClockConfigVersionSyncRace is dropped: compute and store of the
version have no point a test can hold one goroutine at without adding
a seam to production code, so it never failed on the unfixed code.
@tarakanof

Copy link
Copy Markdown
Owner Author

Re-check replies (fixed in 70902ae and ec1fd4e)

  • Should-fix: rotation skip on a stale cache. Fixed, with both of your options.
    • putClockConfig now reads the clock's list again during the request. It skips the clock write only when that read succeeds and matches the patch.
    • If the read fails, it writes anyway, so the old 502 behaviour stays.
    • handleDeviceAppsPut clears the cache again, without a sync.
    • Tests:
      • TestClockConfigRotationResendAfterOutsideReorderIsWritten runs your 3 steps.
      • TestClockConfigRotationWrittenWhenReadFails.
    • Both fail on 1f49188.
    • The ARCHITECTURE "Rotation" and "Version" paragraphs are updated to match.
  • Nit: TestClockConfigVersionSyncRace. Dropped.
    • On the unfixed code there is no point between compute and store where a test can hold one goroutine.
    • Compose loads the config before taking appsMu. A test that holds appsMu blocks both goroutines in the same place, so their order stays nondeterministic.
    • Making it deterministic would need a hook in production code, so I removed the test rather than keep one that never fails.
    • The fix itself (clockSyncGate.mu serialising compute and store) is unchanged.
  • Nit: my earlier reply was wrong. I said all the new tests fail on 0aebdc4 except the race test. In fact TestLastSeenUpdateLeavesEpochAlone passes on 0aebdc4; it pins existing behaviour. The PutSettings store tests don't compile there. The other new cmd/ember tests, and the regenerated golden, fail on 0aebdc4 as stated.
  • Nit: openapi 500. It now reads "The settings store failed; no settings changed."

go vet passes and go test ./... -race -shuffle=on passes.

@tarakanof

Copy link
Copy Markdown
Owner Author

Re-check 2

Read-only re-check of 70902ae and ec1fd4e (git diff 1f49188..HEAD), run in a scratch copy. go vet and gofmt are clean. go test -race -shuffle=on -count=1 ./... passed 4 out of 4 runs.

No blockers or should-fix findings. The stale-rotation skip is fixed. I found three nits.

nit

  1. No test covers a reorder made on the clock's web UI, so the re-read is only partly pinned.

    • TestClockConfigRotationResendAfterOutsideReorderIsWritten still passes when the re-read is removed: the clear in handleDeviceAppsPut is enough on its own.
    • A mutation that keeps the re-read but compares against the stale snapshot also passes the whole suite (refreshClockRotation(ctx) && rotationUnchanged(next.Rotation, cur.Rotation)).
    • Dropping the old-route clear passes too. The re-read covers that case, so this one is fine.
    • Suggested test: GET the façade, edit stub.apps directly to stand in for a web-UI reorder, then PUT the old rotation and expect one order write.
  2. The clear can add one epoch bump in a narrow case. In the usual flow it adds none: the clear does no resync, seenClock uses version only on create, and the app re-reads right after its write. The extra bump needs all of these, in order:

    1. A PUT /v1/device/apps that leaves the order as it was.
    2. An unrelated resync (tryUpdateConfig, setAppHidden or a reload) before the next list read. It stores the hash with rotation: null.
    3. The next read, which restores the old list and bumps again.

    This is harmless because nothing consumes the clock version yet. Related: a façade GET whose 2 s read fails after an old-route PUT returns rotation: null and a version header that differs from the stored record.

  3. ARCHITECTURE "Version" paragraph.

    • "falls back to the last one" no longer holds after an old-route PUT or a façade write: the fallback is then null.
    • "moves the version once" has the exception described in nit 2.
    • The edited line ("there moves the version once, … (the app re-reads after a") is not rewrapped.
    • Optional: say that the pre-write read uses the same 2 s budget.

Verified, clean

  • The skip needs a fresh match.
    • putClockConfig calls refreshClockRotation (2 s budget) and compares against the list it just loaded. It skips the write only when that read succeeds and equals the patch.
    • A failed read leaves the old cache in place but always writes.
    • TestClockConfigRotationRoundTripsWithoutClockWrite still shows that a fresh match does no write.
  • Failed writes are surfaced.
    • An order-write error returns errClockWrite, which becomes a 502 with nothing else changed.
    • A failed readback after a write logs a warning and replies rotation: null.
  • No clock HTTP under a lock.
    • Both reads and the write run before tryUpdateConfig, with no cfgMu, appsMu or registry mu held.
    • noteClockApps → syncClockConfigVersion returns early while clockSync.paused > 0. The deferred resync takes the locks in the usual order: clockSync.mu → cfgMu → appsMu, then the registry mu.
  • Latency on the lossy link.
    • A rotation PUT now does read (2 s) + write (menuCallTimeout, 8 s) + readback (2 s), at most about 12 s. That is under the 30 s WriteTimeout.
    • The new read adds at most 2 s. When it is lost, the PUT writes anyway, which is the safe direction.
  • The new tests fail on 1f49188. I ran them on a 1f49188 tree with the HEAD test file.
    • TestClockConfigRotationResendAfterOutsideReorderIsWritten: 1 order write, want 2.
    • TestClockConfigRotationWrittenWhenReadFails: 0 order writes, want 1.
    • Reverting the re-read alone makes ...WrittenWhenReadFails fail.
  • Dropping the race test is fine.
    • clockSyncGate.mu still serialises compute and store in syncClockConfigVersion, so a stale hash can't overwrite a newer one.
    • The builder's reason holds: compose loads the config before it takes appsMu, so no test-side lock can force the interleaving without a production hook.
  • openapi 500 now says "no settings changed". The ARCHITECTURE "Rotation" paragraph matches the code.
  • No code comments were added.

@tarakanof
tarakanof merged commit f67705c into main Oct 9, 2026
43 of 49 checks passed
@tarakanof
tarakanof deleted the feat/230-clock-device branch October 9, 2026 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(devices): TC001 as an awtrix-ng registry device with per-device app config facade

1 participant