amadan
The readonly-database hint has to be right, not just helpful
The hint added on this branch replaces SQLite's "attempt to write a readonly database (8)" — the words that cost the reporter time on disk-space and mount-option theories. As written it could send the next person somewhere equally wrong, in two ways. As ROOT it cannot be an ownership problem, and the hint said it was. The four root units drop no capabilities, so root holds CAP_DAC_OVERRIDE and writes the file whatever its owner and mode. "Fix: chown carlos:carlos" was the one action that could not help. Root now gets the causes that remain possible for it: a read-only remount after an I/O error, an immutable attribute, a hot journal or -wal a crash left unreplayable, or a MAC denial. And it stat'd the database alone. The reporter's SECOND failure — db owned by carlos, -wal still owned by root, which is why two of three boxes needed the chown twice — therefore printed "owned by carlos ... must be owned by carlos". A self-contradiction, on the case that motivated the hint. It now stats all three of registryFiles and reports each owner; where every file that exists is already owned by this euid it says so and names the mount and the unit sandbox instead of repeating advice the operator has already taken. Other SQLite errors still read as themselves — a full disk must not be mislabelled as an ownership problem — and that is now pinned for the root wording too. Tests the previous commit message claimed but did not pin: - open and chown are one operation. RunAdopt used to call registry.Open and then chownRegistryToService; -restart-only returns a dozen lines later, so a future reorder would have silently lost the self-healing on the pass that fires most often. openServiceRegistry does both, RunAdopt can no longer open the registry any other way, and a source-level test keeps it that way rather than an ordering assertion that rots. - a chown failure is logged, never fatal. Both arms now have a test: a service user that does not resolve, and a chown the kernel refuses — the second with two files, so it also pins that the loop steps over a failure rather than returning on it. - the negative case for the root wording: root euid, a file owned by someone else, and the hint must not mention chown. chownRegistryFiles and registryOpenHintFor take the euid and the service user as arguments so a test binary that is not root can reach the root arms. The exported helpers still read os.Geteuid(), and the off-root no-op test now also asserts silence: a laptop or a CI runner must not be told about a chown that was never going to happen. Both structural fences are untouched: bootstrap still pre-creates /data/registry.db before arming anything, and every root adopt pass still chowns the db and its sidecars back to carlos. One note added, no code change: a `-root /tmp/carlos-unitstage` run — how live boxes refresh their units — leaves a 0-byte data/registry.db in the staging tree. Harmless while the deploy script copies only etc/, and destructive the day anything rsyncs the whole tree over /. That script lives in the private platform-infrastructure repo, so the constraint is recorded here where the file is created.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XxxmnFjdNxLc3M16ooFG95
4 files changed,
+344
−30
internal/host/agent.go+7 −6internal/host/bootstrap.go+10 −0internal/host/registryowner.go+135 −21internal/host/registryowner_test.go+192 −3
CI passed — run details
diff --git a/internal/host/agent.go b/internal/host/agent.go| index 1713beadf..d2da8db09 100644 |
| --- a/internal/host/agent.go |
| +++ b/internal/host/agent.go |
| @@ -2814,18 +2814,19 @@ func RunAdopt(args []string) { |
| restartOnly := fs.Bool("restart-only", false, "cycle the routes of apps whose restart generation moved, and nothing else: the agent-requested pass (carlos-restart.service)") |
| fs.Usage = cliflag.Usage(fs, "") |
| _ = fs.Parse(args) |
| - reg, err := registry.Open(*regPath) |
| + // openServiceRegistry, not registry.Open: this pass runs as root |
| + // (carlos-adopt/-stage/-restart/-config-sync have no User=), opening the |
| + // registry is what creates its -wal/-shm, and the helper hands them back |
| + // to the user the edge runs as as part of the open. That matters most for |
| + // -restart-only, which returns a dozen lines below — see registryowner.go |
| + // for why a converge on every pass is what it takes. |
| + reg, err := openServiceRegistry(*regPath) |
| if err != nil { |
| if hint := registryOpenHint(*regPath, err); hint != "" { |
| log.Fatalf("adopt: open registry %s: %v\nadopt: %s", *regPath, err, hint) |
| } |
| log.Fatalf("adopt: open registry %s: %v", *regPath, err) |
| } |
| - // This pass runs as root (carlos-adopt/-stage/-restart/-config-sync have |
| - // no User=), and opening the registry is what creates its -wal/-shm. Hand |
| - // them back to the user the edge runs as, on every pass — see |
| - // registryowner.go for why once is not enough. |
| - chownRegistryToService(*regPath) |
| ctx := context.Background() |
| store, err := openDeployStore(ctx) |
| if err != nil { |
diff --git a/internal/host/bootstrap.go b/internal/host/bootstrap.go| index d59aed6f9..a34b66e08 100644 |
| --- a/internal/host/bootstrap.go |
| +++ b/internal/host/bootstrap.go |
| @@ -251,6 +251,16 @@ func bootstrap(root string, consumers []string) error { |
| // as a registry here: on a re-run over a live box this must not touch |
| // the schema of a database the running edge holds open, and "the file |
| // exists, owned by the right user" is the whole job. |
| + // |
| + // One constraint this puts on the deploy side: a STAGING run takes this |
| + // branch too. `carlos bootstrap -root <dir>` is how a live box refreshes |
| + // its unit files (pinfra's update-platform-binary.sh stages into |
| + // /tmp/carlos-unitstage), so a staging tree now contains a 0-byte |
| + // data/registry.db. Harmless while that script copies only etc/ out of |
| + // the tree — and destructive the day anything rsyncs the whole tree over |
| + // /, because a 0-byte file would land on top of the live registry and |
| + // empty it. The script lives in the private platform-infrastructure repo; |
| + // whoever widens what it copies has to exclude data/. |
| regPath := filepath.Join(dataDir, "registry.db") |
| if _, err := os.Stat(regPath); os.IsNotExist(err) { |
| f, err := os.OpenFile(regPath, os.O_CREATE|os.O_WRONLY, 0o640) |
diff --git a/internal/host/registryowner.go b/internal/host/registryowner.go| index e704afd48..756b9e9d0 100644 |
| --- a/internal/host/registryowner.go |
| +++ b/internal/host/registryowner.go |
| @@ -5,8 +5,11 @@ import ( |
| "log" |
| "os" |
| "os/user" |
| + "path/filepath" |
| "strconv" |
| "strings" |
| + |
| + "github.com/carlosframework/platform/internal/registry" |
| ) |
| // The registry is written by TWO users. `carlos agent` (carlos-edge.service) |
| @@ -37,6 +40,32 @@ import ( |
| // path/timer units fire again and re-create the sidecars as root in |
| // between. Two of the three boxes needed the chown twice. |
| +// openServiceRegistry opens the registry for a root-side pass and converges |
| +// its ownership in the same breath. |
| +// |
| +// One function rather than two statements, because the two must not drift. |
| +// Opening the database is what CREATES the -wal/-shm, so the converge has to |
| +// follow the open — and `carlos adopt -restart-only` returns from RunAdopt a |
| +// few lines after it. A converge written as a separate statement that ever |
| +// slid below that early return would silently stop self-healing the restart |
| +// path, which is the pass that fires most often on a live box (carlos-restart |
| +// .path is per-request). Callers get the registry only through here, so there |
| +// is no ordering left to get wrong. |
| +func openServiceRegistry(path string) (*registry.Registry, error) { |
| + return openRegistryConverging(os.Geteuid(), serviceUser, path) |
| +} |
| + |
| +// openRegistryConverging is openServiceRegistry with the two facts it reads |
| +// from the process passed in; see chownRegistryFiles. |
| +func openRegistryConverging(euid int, svcUser, path string) (*registry.Registry, error) { |
| + reg, err := registry.Open(path) |
| + if err != nil { |
| + return nil, err |
| + } |
| + chownRegistryFiles(euid, svcUser, path) |
| + return reg, nil |
| +} |
| + |
| // chownRegistryToService leaves the registry and its WAL sidecars owned by |
| // serviceUser. It is a no-op unless this process is root: a pass running as |
| // the service user already creates everything with the right owner. |
| @@ -48,12 +77,20 @@ import ( |
| func chownRegistryToService(path string) { |
| // Geteuid reports -1 on platforms without the concept (the CLI |
| // cross-compiles to windows), which takes the same early exit. |
| - if os.Geteuid() != 0 { |
| + chownRegistryFiles(os.Geteuid(), serviceUser, path) |
| +} |
| + |
| +// chownRegistryFiles is chownRegistryToService with the process facts — the |
| +// effective uid and the service user's name — made arguments, so a test can |
| +// drive the root arm, and both of its failure arms, from a test binary that |
| +// is not root on a box that has no carlos user. |
| +func chownRegistryFiles(euid int, svcUser, path string) { |
| + if euid != 0 { |
| return |
| } |
| - u, err := user.Lookup(serviceUser) |
| + u, err := user.Lookup(svcUser) |
| if err != nil { |
| - log.Printf("adopt: registry ownership: no %s user: %v", serviceUser, err) |
| + log.Printf("adopt: registry ownership: no %s user: %v", svcUser, err) |
| return |
| } |
| uid, err := strconv.Atoi(u.Uid) |
| @@ -75,13 +112,15 @@ func chownRegistryToService(path string) { |
| continue |
| } |
| if err := os.Chown(p, uid, gid); err != nil { |
| - log.Printf("adopt: chown %s to %s: %v", p, serviceUser, err) |
| + // Logged and stepped over, never returned: the files after this |
| + // one may well be fixable, and none of it is worth the pass. |
| + log.Printf("adopt: chown %s to %s: %v", p, svcUser, err) |
| continue |
| } |
| // Worth a line every time it fires: on a healthy box it never does, |
| // so a run of these is the box telling you something armed root is |
| // creating the registry ahead of the edge. |
| - log.Printf("adopt: %s was not owned by %s — fixed; the edge runs as %s and could not have written it", p, serviceUser, serviceUser) |
| + log.Printf("adopt: %s was not owned by %s — fixed; the edge runs as %s and could not have written it", p, svcUser, svcUser) |
| } |
| } |
| @@ -96,28 +135,103 @@ func registryFiles(path string) []string { |
| // |
| // "attempt to write a readonly database (8)" is what the edge printed on all |
| // three boxes, and it sent the operator looking at disk space and mount |
| -// options before file ownership — which is the actual cause every time so far, |
| -// because the registry is the one file on the box two different users write. |
| -// The hint names the owner it found, the owner it needs, and the command that |
| -// fixes it. It returns "" for any other error, so a genuinely full disk still |
| -// reads as itself. |
| +// options before file ownership. The hint exists so the next person does not |
| +// lose that time — which means it has to be RIGHT and not merely helpful: a |
| +// hint that names ownership where ownership is not the cause sends them |
| +// somewhere just as wrong, with more confidence behind it. |
| +// |
| +// So it says one of three things, and the process's own euid picks which: |
| +// |
| +// - As ROOT, ownership is never the answer. The root units drop no |
| +// capabilities, so root holds CAP_DAC_OVERRIDE and writes the file |
| +// whatever its owner and mode; a chown is the one action that cannot |
| +// help. What remains possible for root is a read-only remount, an |
| +// immutable attribute, a hot journal, or a MAC denial — and those are |
| +// what it names. |
| +// - Off root, holding a file this process does not own, ownership IS the |
| +// answer and it gives the chown. |
| +// - Off root with everything already owned correctly, ownership is not the |
| +// answer either, so it points at the mount and the unit sandbox rather |
| +// than repeating advice the operator has already taken. |
| +// |
| +// It stats all three of registryFiles rather than the database alone. A |
| +// carlos-owned database with a root-owned -wal reports the identical error, |
| +// and that is exactly the shape a hand chown of the database alone leaves |
| +// behind — the second failure in #8, and why two of the three boxes needed |
| +// the chown twice. Reporting the database alone printed "owned by carlos … |
| +// must be owned by carlos" there: a self-contradiction, on the very case that |
| +// motivated the hint. |
| +// |
| +// It returns "" for any other error, so a genuinely full disk still reads as |
| +// itself. |
| func registryOpenHint(path string, err error) string { |
| + return registryOpenHintFor(os.Geteuid(), path, err) |
| +} |
| + |
| +// registryOpenHintFor is registryOpenHint with the effective uid passed in, |
| +// so the root and non-root wordings are both reachable from one test binary. |
| +func registryOpenHintFor(euid int, path string, err error) string { |
| if err == nil || !strings.Contains(strings.ToLower(err.Error()), "readonly database") { |
| return "" |
| } |
| var b strings.Builder |
| - fmt.Fprintf(&b, "the registry is not writable by this process (uid %d).", os.Geteuid()) |
| - if fi, statErr := os.Stat(path); statErr == nil { |
| - if uid, gid, ok := fileOwner(fi); ok { |
| - owner := strconv.Itoa(uid) |
| - if u, lookupErr := user.LookupId(owner); lookupErr == nil { |
| - owner = u.Username |
| - } |
| - fmt.Fprintf(&b, " %s is owned by %s (uid %d, gid %d)", path, owner, uid, gid) |
| + fmt.Fprintf(&b, "the registry is not writable by this process (uid %d).", euid) |
| + |
| + // present counts the files that exist; mine, the ones this process owns. |
| + // They are equal only when there is nothing left for a chown to fix. |
| + var present, mine int |
| + for _, p := range registryFiles(path) { |
| + fi, statErr := os.Stat(p) |
| + if statErr != nil { |
| + // A missing -wal/-shm is the normal state between connections, |
| + // not a finding to report. |
| + continue |
| + } |
| + present++ |
| + uid, gid, ok := fileOwner(fi) |
| + if !ok { |
| + // Off unix there is no owner to read. Counted as present but |
| + // never as mine, so the hint never claims ownership is fine on |
| + // evidence it does not have. |
| + fmt.Fprintf(&b, " %s exists (owner unreadable on this platform).", p) |
| + continue |
| } |
| + if uid == euid { |
| + mine++ |
| + } |
| + fmt.Fprintf(&b, " %s is owned by %s (uid %d, gid %d).", p, ownerName(uid), uid, gid) |
| + } |
| + |
| + dir := filepath.Dir(path) |
| + switch { |
| + case euid == 0: |
| + fmt.Fprintf(&b, " Running as root, this is NOT an ownership problem: the root units drop"+ |
| + " no capabilities, so root holds CAP_DAC_OVERRIDE and writes the file whatever its"+ |
| + " owner and mode — changing the owner would change nothing. What can still produce"+ |
| + " it for root: the filesystem remounted read-only after an I/O error"+ |
| + " (`dmesg -T | tail`, `findmnt -no OPTIONS %s`), an immutable attribute on one of"+ |
| + " the files (`lsattr %s*`), a hot journal or -wal left by a crash that SQLite"+ |
| + " cannot replay, or a MAC denial (SELinux/AppArmor) on the write.", dir, path) |
| + case present > 0 && present == mine: |
| + fmt.Fprintf(&b, " Ownership is already correct — every file that exists is owned by this"+ |
| + " uid — so setting it again will not help. Look instead at the mount (a read-only"+ |
| + " remount after an I/O error, or a read-only %s: `findmnt -no OPTIONS %s`) and at"+ |
| + " this unit's sandbox (ProtectSystem= or ReadOnlyPaths=, or %s missing from"+ |
| + " ReadWritePaths=).", dir, dir, dir) |
| + default: |
| + fmt.Fprintf(&b, " It and its -wal/-shm sidecars must be owned by %s, the user"+ |
| + " carlos-edge.service runs as. A root `carlos adopt` pass that reached the file"+ |
| + " before the edge did is the usual cause. Fix: chown %s:%s %s*", |
| + serviceUser, serviceUser, serviceUser, path) |
| } |
| - fmt.Fprintf(&b, " — it and its -wal/-shm sidecars must be owned by %s, the user carlos-edge.service runs as."+ |
| - " A root `carlos adopt` pass that reached the file before the edge did is the usual cause."+ |
| - " Fix: chown %s:%s %s*", serviceUser, serviceUser, serviceUser, path) |
| return b.String() |
| } |
| + |
| +// ownerName resolves a uid to a username, falling back to the number itself. |
| +// A box whose passwd lookup is broken still gets a usable hint. |
| +func ownerName(uid int) string { |
| + if u, err := user.LookupId(strconv.Itoa(uid)); err == nil && u.Username != "" { |
| + return u.Username |
| + } |
| + return strconv.Itoa(uid) |
| +} |
diff --git a/internal/host/registryowner_test.go b/internal/host/registryowner_test.go| index 48e7d22a3..10a0d43d6 100644 |
| --- a/internal/host/registryowner_test.go |
| +++ b/internal/host/registryowner_test.go |
| @@ -113,6 +113,7 @@ func TestChownRegistryToServiceIsANoopOffRoot(t *testing.T) { |
| if err != nil { |
| t.Fatal(err) |
| } |
| + logged := captureLog(t) |
| chownRegistryToService(path) // must not panic, must not change anything |
| after, err := os.Stat(path) |
| if err != nil { |
| @@ -123,6 +124,11 @@ func TestChownRegistryToServiceIsANoopOffRoot(t *testing.T) { |
| if ok != ok2 || buid != auid || bgid != agid { |
| t.Errorf("ownership moved off root: %d:%d -> %d:%d", buid, bgid, auid, agid) |
| } |
| + // Silent, too: a laptop or a CI runner must not be told about a chown it |
| + // was never going to do. |
| + if out := logged(); out != "" { |
| + t.Errorf("off root the helper logged:\n%s", out) |
| + } |
| } |
| // TestRegistryFilesCoversTheWALSidecars: a carlos-owned database with a |
| @@ -142,16 +148,22 @@ func TestRegistryFilesCoversTheWALSidecars(t *testing.T) { |
| } |
| } |
| +// foreignUID is a uid that is certainly not the one owning a file this test |
| +// process just created, and certainly not root. It is how the "somebody else |
| +// owns the registry" shape is reached from a test binary that cannot chown. |
| +func foreignUID() int { return os.Geteuid() + 1 } |
| + |
| // TestRegistryOpenHintNamesOwnership is the cheap fence #8 asked for: |
| // "attempt to write a readonly database (8)" is SQLite's words, and they sent |
| -// the operator looking at disk space and mount options. The hint has to name |
| -// ownership, the service user, and the command that fixes it. |
| +// the operator looking at disk space and mount options. Where ownership IS |
| +// the cause — a non-root process holding a file it does not own — the hint |
| +// has to name the owner it found, the service user, and the fixing command. |
| func TestRegistryOpenHintNamesOwnership(t *testing.T) { |
| path := filepath.Join(t.TempDir(), "registry.db") |
| if err := os.WriteFile(path, nil, 0o640); err != nil { |
| t.Fatal(err) |
| } |
| - hint := registryOpenHint(path, errors.New("attempt to write a readonly database (8)")) |
| + hint := registryOpenHintFor(foreignUID(), path, errors.New("attempt to write a readonly database (8)")) |
| if hint == "" { |
| t.Fatal("no hint for the readonly-database error") |
| } |
| @@ -162,6 +174,83 @@ func TestRegistryOpenHintNamesOwnership(t *testing.T) { |
| } |
| } |
| +// TestRegistryOpenHintStatsTheSidecars: the hint used to stat the database |
| +// alone, which is why the reporter's SECOND failure — database owned by |
| +// carlos, -wal still owned by root — printed "owned by carlos … must be owned |
| +// by carlos". Every file that exists has to be reported, or the hint |
| +// contradicts itself on the exact case that motivated it. |
| +func TestRegistryOpenHintStatsTheSidecars(t *testing.T) { |
| + dir := t.TempDir() |
| + path := filepath.Join(dir, "registry.db") |
| + for _, p := range registryFiles(path) { |
| + if err := os.WriteFile(p, nil, 0o640); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + hint := registryOpenHintFor(foreignUID(), path, errors.New("attempt to write a readonly database (8)")) |
| + for _, p := range registryFiles(path) { |
| + if !strings.Contains(hint, p) { |
| + t.Errorf("hint says nothing about %s:\n%s", p, hint) |
| + } |
| + } |
| +} |
| + |
| +// TestRegistryOpenHintDoesNotBlameOwnershipAsRoot is the negative fence. The |
| +// root passes drop no capabilities, so root holds CAP_DAC_OVERRIDE and writes |
| +// a file whatever its owner — a "readonly database" seen by root can NEVER be |
| +// an ownership problem, and telling root to chown is pointing at the one |
| +// action that cannot help. Note the file here is owned by somebody other than |
| +// root as far as the hint is concerned: even with a mismatch in front of it, |
| +// the root wording must hold. |
| +func TestRegistryOpenHintDoesNotBlameOwnershipAsRoot(t *testing.T) { |
| + path := filepath.Join(t.TempDir(), "registry.db") |
| + if err := os.WriteFile(path, nil, 0o640); err != nil { |
| + t.Fatal(err) |
| + } |
| + hint := registryOpenHintFor(0, path, errors.New("attempt to write a readonly database (8)")) |
| + if hint == "" { |
| + t.Fatal("no hint for the readonly-database error") |
| + } |
| + if strings.Contains(hint, "chown") { |
| + t.Errorf("the root hint tells the operator to chown, which cannot help root:\n%s", hint) |
| + } |
| + // And it names causes that ARE possible for root. |
| + for _, want := range []string{"CAP_DAC_OVERRIDE", "read-only", "immutable", "SELinux"} { |
| + if !strings.Contains(hint, want) { |
| + t.Errorf("the root hint does not mention %q:\n%s", want, hint) |
| + } |
| + } |
| +} |
| + |
| +// TestRegistryOpenHintSaysNothingIsWrongWithOwnership: off root, with every |
| +// file that exists already owned by this process, ownership is not the cause |
| +// either. Repeating the chown is exactly the advice the reporter had already |
| +// followed; the hint has to move on to the mount and the unit sandbox. |
| +func TestRegistryOpenHintSaysNothingIsWrongWithOwnership(t *testing.T) { |
| + if os.Geteuid() == 0 { |
| + t.Skip("running as root: the root wording is pinned by its own test") |
| + } |
| + dir := t.TempDir() |
| + path := filepath.Join(dir, "registry.db") |
| + for _, p := range registryFiles(path) { |
| + if err := os.WriteFile(p, nil, 0o640); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + hint := registryOpenHintFor(os.Geteuid(), path, errors.New("attempt to write a readonly database (8)")) |
| + if hint == "" { |
| + t.Fatal("no hint for the readonly-database error") |
| + } |
| + if strings.Contains(hint, "chown") { |
| + t.Errorf("ownership is already correct and the hint still asks for a chown:\n%s", hint) |
| + } |
| + for _, want := range []string{"already correct", "mount", "ReadWritePaths"} { |
| + if !strings.Contains(hint, want) { |
| + t.Errorf("hint does not mention %q:\n%s", want, hint) |
| + } |
| + } |
| +} |
| + |
| // TestRegistryOpenHintSilentOnOtherErrors: a genuinely full disk or a missing |
| // directory must still read as itself. The hint is only for the one error it |
| // can actually explain. |
| @@ -175,5 +264,105 @@ func TestRegistryOpenHintSilentOnOtherErrors(t *testing.T) { |
| if hint := registryOpenHint(path, err); hint != "" { |
| t.Errorf("registryOpenHint(%v) = %q, want no hint", err, hint) |
| } |
| + // Both wordings that DO fire have to stay off these errors too. |
| + if hint := registryOpenHintFor(0, path, err); hint != "" { |
| + t.Errorf("registryOpenHintFor(root, %v) = %q, want no hint", err, hint) |
| + } |
| + } |
| +} |
| + |
| +// TestOpenServiceRegistryConvergesOwnership pins the two halves as one |
| +// operation. `carlos adopt -restart-only` returns from RunAdopt within a few |
| +// lines of the open, so a converge that ever slid below that early return |
| +// would silently stop self-healing the pass that fires most often. Opening |
| +// through this helper is what makes the ordering unloseable — the test drives |
| +// the root arm with a service user that cannot resolve, so the one thing it |
| +// observes is that the open reached the converge at all. |
| +func TestOpenServiceRegistryConvergesOwnership(t *testing.T) { |
| + path := filepath.Join(t.TempDir(), "registry.db") |
| + logged := captureLog(t) |
| + const noSuchUser = "carlos-no-such-service-user-8f21" |
| + reg, err := openRegistryConverging(0, noSuchUser, path) |
| + if err != nil { |
| + t.Fatalf("open: %v", err) |
| + } |
| + defer reg.Close() |
| + if _, err := reg.List(); err != nil { |
| + t.Fatalf("the helper returned an unusable registry: %v", err) |
| + } |
| + if !strings.Contains(logged(), noSuchUser) { |
| + t.Errorf("opening the registry did not run the ownership converge; log was:\n%s", logged()) |
| } |
| } |
| + |
| +// TestRunAdoptOpensTheRegistryThroughTheHelper is the other half of the same |
| +// fence, and the reason it reads source rather than asserting an order: an |
| +// ordering assertion rots the moment the function is rearranged, while "the |
| +// root pass never calls registry.Open itself" stays true for as long as the |
| +// invariant does. |
| +func TestRunAdoptOpensTheRegistryThroughTheHelper(t *testing.T) { |
| + src, err := os.ReadFile("agent.go") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + i := strings.Index(string(src), "\nfunc RunAdopt(") |
| + if i < 0 { |
| + t.Fatal("RunAdopt is no longer in agent.go — move this test with it") |
| + } |
| + rest := string(src)[i+1:] |
| + j := strings.Index(rest, "\n}\n") |
| + if j < 0 { |
| + t.Fatal("could not find the end of RunAdopt") |
| + } |
| + body := rest[:j] |
| + if !strings.Contains(body, "openServiceRegistry(") { |
| + t.Error("RunAdopt no longer opens the registry through openServiceRegistry — the root pass has stopped converging ownership") |
| + } |
| + if strings.Contains(body, "registry.Open(") { |
| + t.Error("RunAdopt opens the registry directly; use openServiceRegistry so the chown cannot be lost ahead of the -restart-only early return") |
| + } |
| +} |
| + |
| +// TestChownRegistryFailureIsLoggedNotFatal: an adoption pass that can read the |
| +// registry has releases to stage and units to cycle. A chown it cannot do is |
| +// a cosmetic problem, and dying over it would turn that into an outage — so |
| +// every failure arm logs and carries on. A test that reaches its own end is |
| +// the assertion: log.Fatalf would take the process with it. |
| +func TestChownRegistryFailureIsLoggedNotFatal(t *testing.T) { |
| + t.Run("service user does not resolve", func(t *testing.T) { |
| + path := filepath.Join(t.TempDir(), "registry.db") |
| + if err := os.WriteFile(path, nil, 0o640); err != nil { |
| + t.Fatal(err) |
| + } |
| + logged := captureLog(t) |
| + chownRegistryFiles(0, "carlos-no-such-service-user-8f21", path) |
| + if !strings.Contains(logged(), "carlos-no-such-service-user-8f21") { |
| + t.Errorf("the failed lookup was not logged:\n%s", logged()) |
| + } |
| + }) |
| + |
| + t.Run("the chown itself is refused", func(t *testing.T) { |
| + if os.Geteuid() == 0 { |
| + t.Skip("running as root: chown would succeed, so there is no refusal to observe") |
| + } |
| + dir := t.TempDir() |
| + path := filepath.Join(dir, "registry.db") |
| + // Database and -wal both present and both owned by this (non-root) |
| + // user: chowning them to root is refused for each in turn. |
| + for _, p := range []string{path, path + "-wal"} { |
| + if err := os.WriteFile(p, nil, 0o640); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + logged := captureLog(t) |
| + chownRegistryFiles(0, "root", path) |
| + out := logged() |
| + // Both files reported, which is the "continue, do not return" half: |
| + // the loop stepped over the first failure and tried the second. |
| + for _, p := range []string{path, path + "-wal"} { |
| + if !strings.Contains(out, "chown "+p+" to root") { |
| + t.Errorf("no logged failure for %s; log was:\n%s", p, out) |
| + } |
| + } |
| + }) |
| +} |