Repository navigation
fix: stop warning about normal things on a fresh start - #539
Merged
Merged
Conversation
Both libraries print to stderr through their own loggers by default, so a failing Redis sample check wrote raw "redis: ... pool.go:770" lines outside the log format. Route them into slog at DEBUG: the failure already reaches the check result. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
NewServer read and wrote app_settings before the migrations created it. A fresh database logged two "no such table" SQL warnings and a webpush warning, and served a key that was never stored, so the first restart replaced it and broke every subscription made during the first run. - Load the keys in Server.Initialize, after the migrations (InitializeWebPush, also called by the scenario harness). - GetOrCreateVAPIDKeys never returns a pair that is not stored: read and write errors are returned, both keys are read in one statement, a new pair is inserted with ON CONFLICT DO NOTHING and re-read, so processes racing on a fresh database share one pair. Half a pair is replaced. - dash0 drops a push subscription bound to another key before subscribing again. Also on the server boot/shutdown path: - One accurate /metrics line instead of "disabled" followed by "enabled". - No "Sentry flush timed out" warning when Sentry was never initialized. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…can collide The check worker and the job worker each warned that the hostname was cut to build the worker slug, so a laptop hostname such as alices-macbook-air.local printed the same WARN twice on every start. The cut only matters when another process can register in the same database: a role other than `all`, or an external Postgres. Log it once per process, at WARN in that case and at DEBUG on a single node. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- The audit cleanup job warned "Using deprecated koanf aggregation retention" for audit.retention_days on every install: config.Load fills that value with the default, and the key is not deprecated. The warning now fires once at boot, only when a config file sets one of the deprecated aggregation.retention_* keys. - "Configuration loaded" logs runMode=production instead of an empty string. - The first-boot JWT secret generation is expected, log it at INFO. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #539 +/- ##
========================================
Coverage 82.74% 82.74%
========================================
Files 1175 1178 +3
Lines 121460 121576 +116
========================================
+ Hits 100497 100602 +105
- Misses 20956 20967 +11
Partials 7 7
🚀 New features to boost your workflow:
|
This was referenced Oct 12, 2026
fclairamb
added a commit
that referenced
this pull request
Oct 12, 2026
#539 implemented the same VAPID spec (2026-10-12-03) on main. Its version is kept for every file this branch only touched for that spec (webpush, db app_settings, server.go, scenario harness, dash0 subscription hook, notifications mock); the branch's duplicate implementation and its useWebPushSubscription.test.ts are dropped. #537 (SeedOnInstall) and the idle-memory spec (NeedsBrowser) both add a CheckSpec flag: both are kept. A fresh default install now seeds no browser sample, so TestStartupBrowserSamplesDisabled runs in test mode, with http-fake-stable as the enabled control, and the small-hosts docs say so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fclairamb
added a commit
that referenced
this pull request
Oct 12, 2026
#539 implemented the same VAPID spec (2026-10-12-03) on main. Its version is kept for every file this branch only touched for that spec (webpush, db app_settings, server.go, scenario harness, dash0 subscription hook, notifications mock); the branch's duplicate implementation and its useWebPushSubscription.test.ts are dropped. #537 (SeedOnInstall) and the idle-memory spec (NeedsBrowser) both add a CheckSpec flag: both are kept. A fresh default install now seeds no browser sample, so TestStartupBrowserSamplesDisabled runs in test mode, with http-fake-stable as the enabled control, and the small-hosts docs say so. Co-Authored-By: Claude Opus 5.5 <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
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.
Problem
A fresh server (SQLite, empty data dir, no config) looks broken in its first minute of logs:
The VAPID one is a real bug, not only noise: the key served during the first run was never stored, so the first restart generated a new one and every browser subscribed in between stopped receiving pushes.
Fix
Server.Initialize, after the migrations (InitializeWebPush, also called by the scenario harness that skipsInitialize).GetOrCreateVAPIDKeysnever returns a pair that is not stored: read and write errors are returned, both keys are read in one statement, a new pair is inserted withON CONFLICT DO NOTHINGand re-read, so processes racing on a fresh database share one pair. Half a stored pair is replaced in one statement. Newdb.Servicemethods:GetAppSettings,CreateAppSettingsIfAbsent,SetAppSettings. dash0 now drops a push subscription bound to another key before subscribing again, so a browser bitten by the old bug can re-enable push.audit.retention_dayson every install, becauseconfig.Loadfills that value with the default (and that key is not deprecated). The job no longer warns.config.Loadwarns once at boot when a config file sets one of the deprecatedaggregation.retention_*keys, never for a default.all, or an external Postgres). A singleallnode on SQLite or embedded Postgres logs it at DEBUG. Docs updated.metrics endpoint disabled, set metrics.scrape_token to enable scraping path=/metrics. The contradicting "enabled ... tokenConfigured=false" line is gone.Configuration loaded runMode=productioninstead ofrunMode="".WARN credentials encryption disabledis kept: it is real security advice.After this PR, a fresh 65 s run prints one WARN (
credentials encryption disabled), no ERROR, and no line outside slog.Found and left alone:
SP_AUDIT_RETENTION_DAYS=0is documented as "keep forever", but0is rejected by the env tier (Ignoring invalid retention env override) and skipped byapplyAuditEnv, so the 365-day default still applies. A negative value does disable the sweep. Separate fix.os.Stderr, with no logger hook. Only Oracle checks hit it.log.Printf, which already goes through slog (at INFO).Test plan
go test -short ./...inserver/(249 packages, all ok)go test -run Postgres ./internal/webpush/(new embedded Postgres test: unmigrated error, concurrent creation, restart)WorkerSlugMayCollide,RunModeName, silent Sentry flush, single metrics linegolangci-lint run ./...(0 issues)vitest run(154 files, 2533 tests) and eslint on the changed filesenv -i ... SP_DB_TYPE=sqlite SP_DB_DIR=$(mktemp -d) ./sp serve): only the encryption WARN left, no raw output. WithSP_LOG_LEVEL=debug, the go-redis lines and the hostname line show up once as DEBUG slog records.🤖 Generated with Claude Code