Plugins: create the settings collection instead of waiting for it forever
01a8fecfixed the advice that led operators into this, but advice is not a guard: a stack still running PB_BOOTSTRAP=false gets no app_settings collection on upgrade, and the plugin panel sits at 503 while the retry loop reads a collection that does not exist. The fix is not to soften the reading. A missing collection stays "not ready" rather than "no plugins configured", because the alternative lets the first save write a fresh document over settings the server merely failed to find - the failure this whole line of work exists to prevent. Instead the server now fixes the cause: on a missing collection it creates that collection and reads again. Three pieces: bootstrap.EnsureCollection creates one named collection from the desired schema if absent, and nothing else. Deliberately narrower than Run - no field reconcile elsewhere, no super-admin - so it is safe to call on a deployment that turned the full bootstrap off. It creates the collection the server cannot start without, not the schema the operator declined. The store tells a missing collection apart from an outage. A 404 from a list means the collection itself is gone: an existing but empty one answers 200 with no items. That is tagged errNoCollection, which wraps errNotReady so every write is still refused, and IsMissingCollection narrows it. The distinction matters because the remedies are opposites - creating collections against a flaky database is exactly the wrong reflex, and a test pins that an outage does not trigger it. loadPlugins acts on the tag once, then re-reads. Failing to create is reported as the original read error rather than the repair's, so the log names the real problem. Six tests: the tag and its negative in internal/plugins, and three in internal/api against a fake PocketBase covering the collection being created exactly once, an existing collection not being recreated, and an outage creating nothing. Docs from01a8fecare corrected in the same pass - they said the panel would answer 503 forever, which is no longer true. They now say what still depends on the bootstrap (every other collection and field) and what does not (app_settings alone). go build, go vet and go test ./... pass; compose files still parse. Not verified: no Docker CLI here, so the repair has not been exercised against a real PocketBase, only the fake. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
01a8fecf40
commit
660af5736a
@@ -263,6 +263,11 @@ it runs as its own process/container, an external plugin is also the
|
||||
- **Upgrading from a pre-PocketBase install**: an existing `plugins.json` (see
|
||||
`PLUGINS_FILE`) is imported into the database on the first boot that finds no
|
||||
settings there, and renamed to `plugins.json.migrated`.
|
||||
- **If the `app_settings` collection is missing** — an upgrade on a stack that
|
||||
runs with `PB_BOOTSTRAP=false`, so the on-boot schema pass never created it —
|
||||
the server creates that one collection itself and reads again. A missing
|
||||
collection is told apart from a database that is merely unreachable, because
|
||||
the remedy differs: creating collections is the wrong reflex during an outage.
|
||||
- **Secrets** (`Secret: true` fields) are returned masked. On save, a field still
|
||||
equal to the mask keeps its stored value; send a new value to change it, or an
|
||||
empty string to clear it.
|
||||
|
||||
@@ -576,6 +576,12 @@ var (
|
||||
errUnknown = errors.New("unknown plugin")
|
||||
errPersist = errors.New("plugin settings could not be saved")
|
||||
errNotReady = errors.New("plugin settings are not loaded yet")
|
||||
|
||||
// errNoCollection is a not-ready that waiting alone will never resolve: the
|
||||
// collection the settings live in does not exist. It wraps errNotReady, so
|
||||
// the safety behaviour (refuse every write) is unchanged; the extra tag only
|
||||
// lets the caller fix the cause instead of retrying forever.
|
||||
errNoCollection = fmt.Errorf("%w: the settings collection does not exist", errNotReady)
|
||||
)
|
||||
|
||||
// IsUnknown reports whether err came from addressing a plugin that doesn't exist.
|
||||
@@ -590,3 +596,8 @@ func IsPersist(err error) bool { return errors.Is(err, errPersist) }
|
||||
// the store was unreachable at boot and is still being retried. Callers should
|
||||
// answer 503 rather than present the plugin list as empty.
|
||||
func IsNotReady(err error) bool { return errors.Is(err, errNotReady) }
|
||||
|
||||
// IsMissingCollection reports whether err means the settings collection does not
|
||||
// exist. It implies IsNotReady, and narrows it: retrying cannot help, so the
|
||||
// caller should create the collection and read again.
|
||||
func IsMissingCollection(err error) bool { return errors.Is(err, errNoCollection) }
|
||||
|
||||
@@ -103,8 +103,15 @@ func (s *pbStore) find(ctx context.Context) (settingsRecord, bool, error) {
|
||||
|
||||
res, err := s.client.List(ctx, s.collection, q)
|
||||
if err != nil {
|
||||
// A missing collection means bootstrap has not run yet — still "not
|
||||
// ready" rather than "no settings", so nothing gets overwritten.
|
||||
if isNotFound(err) {
|
||||
// A 404 from a list means the collection itself is absent: listing an
|
||||
// existing but empty collection answers 200 with no items. Tagged
|
||||
// apart from a plain outage because retrying a read against a
|
||||
// collection that does not exist can never succeed — the caller has
|
||||
// to create it. Still a flavour of not-ready, so nothing is
|
||||
// overwritten in the meantime.
|
||||
return settingsRecord{}, false, fmt.Errorf("%w: %v", errNoCollection, err)
|
||||
}
|
||||
return settingsRecord{}, false, fmt.Errorf("%w: %v", errNotReady, err)
|
||||
}
|
||||
var items []settingsRecord
|
||||
|
||||
@@ -192,17 +192,35 @@ func TestPBStoreUnreachableStaysUnloaded(t *testing.T) {
|
||||
}
|
||||
|
||||
// A missing collection (bootstrap has not run) is "not ready", never "empty" —
|
||||
// otherwise the first save would write a fresh document over nothing.
|
||||
// otherwise the first save would write a fresh document over nothing. It is also
|
||||
// tagged separately, because retrying alone can never resolve it.
|
||||
func TestPBStoreMissingCollectionIsNotReady(t *testing.T) {
|
||||
m := NewManager(newPBStore(t, &fakePB{missingCol: true}))
|
||||
if err := m.Load(context.Background()); !IsNotReady(err) {
|
||||
err := m.Load(context.Background())
|
||||
if !IsNotReady(err) {
|
||||
t.Fatalf("expected not-ready for a missing collection, got %v", err)
|
||||
}
|
||||
if !IsMissingCollection(err) {
|
||||
t.Fatalf("a missing collection must be distinguishable, got %v", err)
|
||||
}
|
||||
if m.Ready() {
|
||||
t.Fatal("manager must not be ready without its collection")
|
||||
}
|
||||
}
|
||||
|
||||
// A plain outage must NOT look like a missing collection: creating the
|
||||
// collection is not the remedy for a database that is merely unreachable.
|
||||
func TestPBStoreOutageIsNotAMissingCollection(t *testing.T) {
|
||||
m := NewManager(newPBStore(t, &fakePB{listErr: true}))
|
||||
err := m.Load(context.Background())
|
||||
if !IsNotReady(err) {
|
||||
t.Fatalf("expected not-ready, got %v", err)
|
||||
}
|
||||
if IsMissingCollection(err) {
|
||||
t.Fatalf("an outage must not be reported as a missing collection: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
// The legacy file is imported into PocketBase exactly once.
|
||||
func TestPBStoreLegacyImport(t *testing.T) {
|
||||
f := &fakePB{}
|
||||
|
||||
Reference in New Issue
Block a user