amadan
Eighteenth replicator build: the link's directory opened as a directory and nothing else, the long-target fixture sized from the temp root
Review seventeen (Opus; Astra's report pending) found two defects in the seventeenth build's fix for Astra 5. The directory a relative link target is looked at from was opened by path with Go's default flags, so a FIFO a tenant swapped into its place would block the watch and Close behind it, and a device there would have had its driver's open run: it is now opened with O_DIRECTORY|O_NONBLOCK|O_NOCTTY|O_CLOEXEC and anything but a directory is refused by the open itself. The long-target fixture's guard held only under a long temp root, which is why the gate under /var/tmp/ck and Amadan CI both failed on bffc76b5: the target is now sized from the directory the run's temp root gives. The false "pass under /tmp" claim in the review- sixteen dispositions is marked as such. Three mutants killed (default flags; O_DIRECTORY alone dropped; the fixed size under /var/tmp/ck); ops and registry packages pass.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
6 files changed,
+184
−15
docs/superpowers/specs/DISPOSITIONS-metered-replicator.md+15 −1docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review17.md+88 −0internal/ops/linktarget_unix.go+17 −9internal/ops/linktarget_unix_test.go+50 −0internal/ops/litestream_owners.go+2 −1internal/ops/litestream_owners_test.go+12 −4
CI passed — run details
diff --git a/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md b/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md| index f92623bd0..812016dfc 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md |
| @@ -330,4 +330,18 @@ Reports: `OPUS-REVIEW-metered-replicator-review16.md`, `CODEX-FINDINGS-metered-r |
| | Claims (both): the fixture comment "through a `..`"; this file's rows ("the kernel's resolution", "fails with the owned line raw", Astra's numbering, "x/.." in a Reproduction column). | Corrected in place. | this file, tests | | |
| | Astra 1 to 4, 6 to 10, the rest of 11: open as before. | | | | |
| -Package tests pass under `/tmp` and `~/.cache` (the author's runs, not as root). The full gate on this tip is next; the seventeenth build has not been read by a reviewer. |
| +The seventeenth build's claim here that its package tests passed under `/tmp` was false: the long-target fixture it added passed only under a temp root of about 34 bytes or more (the author's `~/.cache/tmp/claude-1001`), and the full gate on `bffc76b5` (`/var/tmp/ck`) and Amadan CI on the same tip both failed on that fixture's own guard, as review seventeen (Opus P2-2) had worked out from the arithmetic. Corrected in the eighteenth build, below. |
| + |
| +## Review seventeen (reviewed `bffc76b5`; Opus accepted with fixes; Astra's report pending at the time of the eighteenth build) |
| + |
| +Report: `OPUS-REVIEW-metered-replicator-review17.md` (Astra's is copied in when it lands, and answered in the next section if it names anything new). Opus found review sixteen's findings closed, Astra 5 closed in behaviour and the render parts of Astra 11 closed, and two new defects in the fix for Astra 5. |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Opus P2-1: `statTarget` opened the link's directory by path with Go's default flags: a FIFO a tenant swaps into the directory's place blocks the open until a writer appears (the watch goroutine hangs, Close waits for ever at `watching.Wait()`, `chainClosed` never lets the replacement open), and a device there would have had its driver's open run. | Fixed: the directory is opened with `O_RDONLY\|O_DIRECTORY\|O_NONBLOCK\|O_NOCTTY\|O_CLOEXEC`; anything but a directory is refused by the open itself with ENOTDIR, before any driver's open, and nothing can block on it or become its controlling terminal. `O_NOFOLLOW` is not added: the tree's own root is reached through systemd's state link. The descriptor is still a fresh open by path, not the one the walk listed from (EE2 stays open, stated in the code). | `linktarget_unix.go` `statTarget` | `TestStatTargetRefusesADirectorySwappedForAFIFO`: with Go's default flags the test fails after five seconds ("the open blocked"); with `O_DIRECTORY` alone dropped it fails because the refusal came from the stat through the handed-back descriptor, not the open (the test asserts the `PathError`'s op is `open`). | |
| +| Opus P2-2: the long-target fixture's guard held only under a long temp root; the gate under `/var/tmp/ck` and CI failed on it. | Fixed: the target is sized from the directory the run's temp root gives (`n := (4097-len(deep))/2 + 1`), so the concatenation outgrows the limit under any root while the target alone stays under 4,095 bytes; the guard checks both. | `litestream_owners_test.go` | With the fixed 1,500 repetitions put back, the test fails under `/var/tmp/ck` ("the concatenation is 4074 bytes"); with the fix it passes under `/var/tmp/ck`, `~/.cache/tmp` and the author's root. | |
| +| Opus P3-1: wording wider than the code ("a descriptor for that directory"; "never a concatenation that could outgrow PATH_MAX"; this file's "pass under `/tmp`"). | Corrected: the comment says the directory is resolved again from its path at that moment and a link in its last component is followed; that the argument handed to the kernel is the target alone, the target itself bounded by what a link can hold; and the review-sixteen claim is marked false above. | `linktarget_unix.go`, `litestream_owners.go`, this file | | |
| +| Claims the code contradicts: "a descriptor for the link's directory" (corrected as above); whether CI runs as root is not established by committed source (open, as before). | | | | |
| +| Astra 1 to 4, 6 to 10, approval H's uid binding, the box-wide refusal decision: open as before; Astra 6 has a second instance closed by P2-1's fix and its first (the walk by path) open. | | | | |
| + |
| +Package tests (the two link tests and the package) pass under `/var/tmp/ck` and `~/.cache/tmp` in the author's runs, not as root. The full gate on this tip is next; the eighteenth build has not been read by a reviewer. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review17.md b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review17.md| new file mode 100644 |
| index 000000000..6e5dd7187 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review17.md |
| @@ -0,0 +1,88 @@ |
| +# Review seventeen: feat/carloku-metered-replicator-20261001 at `bffc76b5` |
| + |
| +**Verdict: accept with fixes.** All of review sixteen's findings are closed, and so are Astra 5 and the remaining render parts of Astra 11. The fix for Astra 5 brings in two new problems: |
| +- `statTarget` opens a directory the tenant can swap, using a plain blocking `os.Open`. That is a regression: it can hang Close for every account. |
| +- The new long-target fixture fails whenever the temp root's path is short, including plain `/tmp`. |
| + |
| +I read source only through `git show bffc76b5:`. I ran nothing. |
| + |
| +## Review sixteen, finding by finding |
| + |
| +| Finding | At `bffc76b5` | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Opus P3-1** (the fleet's namespace, not the tenant's) | **Closed.** Both `litestream_owners.go:671-673, 692-696` and `linktarget_unix.go:19` now say "this process's (mount) namespace". The dispositions list the isolated-instance and `PrivateTmp` divergence as open. | Wording only, no test. | |
| +| **Opus T-1: aliased tree root** | **Closed.** `litestream_owners_test.go:739-751`: `x/state -> private/state`, then `cur -> ../d`. | **No.** With the fifteenth build's `filepath.Join`, `x/state/../d` cleans to `x/d`, which is absent, so the link is omitted and the test fails. It does not tell `fstatat` from the concatenation, since both resolve physically. | |
| +| **Opus T-1: render test** (owner triple, `Platform`, Unowned, Problems) | **Closed.** `render` now writes `%t` for `Platform` (`litestream_metered.go:679`). | **No** for each: the raw triple makes `e == f`, dropping `%t` makes `e == g`, and a raw list makes each pair equal. | |
| +| **Astra 5** (concatenation can exceed PATH_MAX) | **Closed in behaviour** (`linktarget_unix.go:33-39`). It opens a regression (P2-1), and its fixture depends on the temp root (P2-2). | **No** with a long temp root: the concatenation hits `ENAMETOOLONG` and the link becomes a problem. With a short root the fixture fails whether or not the fix is there. | |
| +| **Astra claims** (`observe.go` witness wording) | **Closed.** `observe.go:27-31` now says the generation moves on add/remove or any change to the six columns, and the triggers agree. | n/a | |
| +| **Astra 1–4** (pathname opens, replaceable ancestry, a delayed admitted attempt, exit losing doubt) | **Open, unchanged.** These still block wiring. | Yes, as before. | |
| +| **Astra 6–10** | **Open, unchanged.** Astra 6 now has a second instance (P2-1). | Yes, as before. | |
| +| **Astra 11** (other gaps) | **Partly closed.** The render gaps are closed. Still open: the shutdown, watch, interleaving, pre-witness, latch-recheck, `prepareShadowState` and inode items, plus the 300 ms Close check and wall-clock dependence. | Yes, for those. | |
| + |
| +## Findings |
| + |
| +### P2-1. `statTarget` opens a directory the tenant can swap, blocking and following links (EE2; Astra 6 again, now in the last component) |
| + |
| +**Where:** `internal/ops/linktarget_unix.go:33` (`os.Open(dir)`), called from `litestream_owners.go:697`. The hang propagates through `litestream_metered.go:750` (`f.watching.Wait()`) and `:773` (Close's final `lookNow`, which walks every tree). |
| + |
| +The previous code called `os.Stat`, which never opens anything. The new code re-opens `dir` by its path once for every link it finds. It uses Go's default flags: no `O_DIRECTORY`, `O_NONBLOCK` or `O_NOCTTY`, and it follows a link in the last component. |
| + |
| +**Scenario:** |
| +1. Tenant T fills `u/cache/` with 1,000 file links, then loops `renameat2(RENAME_EXCHANGE)` to swap `u/cache` with a FIFO it made at mode 0666. |
| +2. For one link, `Lstat` and `Readlink` see the directory, then `os.Open("u/cache")` meets the FIFO. A read-only open of a FIFO blocks until a writer appears, and T never opens one. |
| +3. The watch goroutine hangs inside `strictTreeDBs`. Close then waits forever at `:750`, or hangs in its own final look at `:773` if the watch was idle. |
| +4. `chainClosed` never lets the replacement open, so every account's replication stops until the process restarts. |
| + |
| +The same window with `u/cache` swapped for a link to `/dev/watchdog` opens the device, if the fleet runs as root. That is a worse form of Astra 6, because the last component is followed too. |
| + |
| +The descriptor is also not the directory the walk listed (EE2). It is resolved again from the path. |
| + |
| +**Smallest fix:** open with `unix.Open(dir, O_RDONLY|O_DIRECTORY|O_NONBLOCK|O_NOCTTY|O_CLOEXEC, 0)`. |
| +- `O_DIRECTORY` refuses a FIFO or device before the driver's open. |
| +- Do not add `O_NOFOLLOW`: the walk's root is legitimately reached through systemd's state link (the T-1 fixture). |
| +- Better, and also closing EE2: open each directory once with `O_DIRECTORY`, then list, `Lstat`, `Readlink` and `fstatat` from that one descriptor. |
| +- Add a fixture where `dir` is a FIFO, called directly through `statTarget`. |
| + |
| +### P2-2. The long-target fixture fails on any short temp root (H; the CI-failure class again) |
| + |
| +**Where:** `litestream_owners_test.go:756-768`. |
| + |
| +The guard at `:766` fails the test unless `len(deep)+1+len(dest) > 4096`. |
| +- `deep` is the `t.TempDir()` path plus 1,005 bytes, and `dest` is 3,011 bytes. |
| +- `t.TempDir()` is `$TMPDIR/TestStrictTreeNamesALinkToAFile<0–10 digits>/NNN`. |
| +- So the guard passes only when `len($TMPDIR)` is at least about 34. |
| + |
| +| `TMPDIR` | Total length | Result | |
| +|---|---|---| |
| +| `/tmp` | 4,058 to 4,067 | Fatal | |
| +| `/var/tmp/ck` (the gate wrapper) | 4,065 to 4,074 | Fatal | |
| +| `/tmp/k`, `/tmp/g` | 4,060 to 4,069 | Fatal | |
| +| `~/.cache/tmp/claude-1001` (35 bytes) | passes | Still fails about 2% of runs, when the random suffix has eight digits or fewer | |
| + |
| +`.amadan/ci.d/10-gate:52` and `Makefile:18` set no `TMPDIR`. CI will very likely fail here. |
| + |
| +**Smallest fix:** size `dest` from `deep`, for example `n := (4097 - len(deep)) / 2 + 1; dest := strings.Repeat("./", n) + "live.sqlite"`, keeping `len(dest)` under 4095. Alternatively, keep adding components until the guard holds. |
| + |
| +### P3-1. Wording wider than the code |
| + |
| +- **`linktarget_unix.go:20-21` and DISPOSITIONS:** "through a descriptor for that directory." It is a descriptor for whatever the path names at open time, not the listed directory (P2-1). |
| +- **DISPOSITIONS:333:** "Package tests pass under `/tmp`." By the arithmetic in P2-2, this test fails under `/tmp`. |
| +- **`linktarget_unix.go:24` and the code comment:** "never a concatenation that could outgrow PATH_MAX". This is true for the argument. The target itself can still be up to PATH_MAX, which is fine and the same as the kernel's limit. |
| + |
| +## Areas A–FF2 |
| + |
| +- **Unchanged from review sixteen's findings:** A to DD2. This build touched only the link-target stat, `render`, two fixtures and the `observe.go` wording. My review-sixteen reading of the chain climb, admission, the witness, the generation triggers, the latch and chain, lineage, the claims, `walkWithoutLinks` and Close's order stands. |
| +- **EE2:** |
| + - **Descriptor:** see P2-1. |
| + - **Middle links:** `fstatat` with a relative path follows links in the middle physically from the descriptor's directory. That is the kernel's resolution of the link body in the fleet's namespace, so it is correct on a stable tree. |
| + - **Errors:** non-`ENOENT` errors stay problems. `ENOTDIR` and `ELOOP` behave as `os.Stat` did. |
| + - **Non-unix fallback** (`!unix`, so Windows): unchanged concatenation. It is harmless, because `sqliteopen_other` refuses there. |
| +- **FF2:** `render` now covers every exported field, quoted or as a bool. `Owned` is sorted at `litestream_owners.go:233`, and the lists are sorted inside `render`. It is therefore one to one over the inventories a fleet builds, up to list order, which is intended. `facts` is unrendered on purpose: it is compared through the inode checks. Nothing parses the rendering; it is compared only at `:666` and `:939`. |
| + |
| +## Claims the code contradicts |
| + |
| +- **"A descriptor for the link's directory"** (`linktarget_unix.go`, DISPOSITIONS review-sixteen row): it is a fresh open by path. |
| +- **"Package tests pass under `/tmp`"** (DISPOSITIONS:333): the fixture guard at `litestream_owners_test.go:766` fails there. |
| +- **The CI user:** whether CI runs as root is still not established by committed source. |
| + |
| +**Verdict: accept with fixes.** Fix P2-1 (`O_DIRECTORY|O_NONBLOCK|O_NOCTTY` or a walk by descriptor) and P2-2 (size the fixture from the temp root). Astra 1–4, approval H's uid binding and the decision on box-wide refusal still block wiring. |
diff --git a/internal/ops/linktarget_unix.go b/internal/ops/linktarget_unix.go| index 1b5faa6f3..02f0d22fa 100644 |
| --- a/internal/ops/linktarget_unix.go |
| +++ b/internal/ops/linktarget_unix.go |
| @@ -17,11 +17,19 @@ type targetKind struct { |
| // statTarget looks at a link's target as the link spells it, in this |
| // process's mount namespace: an absolute target as it is; a relative one |
| -// relative to the directory the link is in, through a descriptor for that |
| -// directory (fstatat), so the kernel resolves every ".." physically, from |
| -// wherever the component before it led, and the spelling handed to it is |
| -// the target alone (never a concatenation that could outgrow PATH_MAX |
| -// while the link itself resolves). The link is never read again. |
| +// relative to the directory the link is in, through a descriptor opened by |
| +// that directory's path at this moment (not the descriptor the walk listed |
| +// from: the directory is resolved again, and a link in its last component |
| +// is followed, as the tree's own root is reached through one), with |
| +// fstatat, so the kernel resolves every ".." physically, from wherever the |
| +// component before it led, and the argument handed to it is the target |
| +// alone, so the link's directory adds nothing to the length the kernel |
| +// sees (the target itself is bounded by what a link can hold). The open |
| +// asks for a directory and nothing else: a FIFO, a device or a file a |
| +// tenant swaps into the directory's place between the listing and this |
| +// look is refused by the kernel before any driver's open (ENOTDIR), and |
| +// nothing here can block on it or become its controlling terminal. The |
| +// link is never read again. |
| func statTarget(dir, dest string) (targetKind, error) { |
| if filepath.IsAbs(dest) { |
| fi, err := os.Stat(dest) |
| @@ -30,13 +38,13 @@ func statTarget(dir, dest string) (targetKind, error) { |
| } |
| return targetKind{dir: fi.IsDir(), regular: fi.Mode().IsRegular()}, nil |
| } |
| - d, err := os.Open(dir) |
| + fd, err := unix.Open(dir, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_NONBLOCK|unix.O_NOCTTY|unix.O_CLOEXEC, 0) |
| if err != nil { |
| - return targetKind{}, err |
| + return targetKind{}, &os.PathError{Op: "open", Path: dir, Err: err} |
| } |
| - defer d.Close() |
| + defer unix.Close(fd) |
| var st unix.Stat_t |
| - if err := unix.Fstatat(int(d.Fd()), dest, &st, 0); err != nil { |
| + if err := unix.Fstatat(fd, dest, &st, 0); err != nil { |
| if err == unix.ENOENT { |
| return targetKind{}, os.ErrNotExist |
| } |
diff --git a/internal/ops/linktarget_unix_test.go b/internal/ops/linktarget_unix_test.go| new file mode 100644 |
| index 000000000..2af42969c |
| --- /dev/null |
| +++ b/internal/ops/linktarget_unix_test.go |
| @@ -0,0 +1,50 @@ |
| +//go:build unix |
| + |
| +package ops |
| + |
| +import ( |
| + "errors" |
| + "os" |
| + "path/filepath" |
| + "testing" |
| + "time" |
| + |
| + "golang.org/x/sys/unix" |
| +) |
| + |
| +// The directory a relative target is looked at from is opened as a |
| +// directory and nothing else: a FIFO a tenant swaps into its place between |
| +// the listing and the look is refused at once (ENOTDIR), where a plain |
| +// open would wait for a writer that never comes and hang the watch and |
| +// Close behind it. (Review seventeen, Opus P2-1.) |
| +func TestStatTargetRefusesADirectorySwappedForAFIFO(t *testing.T) { |
| + root := platformDir(t) |
| + fifo := filepath.Join(root, "cache") |
| + if err := unix.Mkfifo(fifo, 0o666); err != nil { |
| + t.Fatal(err) |
| + } |
| + done := make(chan error, 1) |
| + go func() { |
| + _, err := statTarget(fifo, "live.sqlite") |
| + done <- err |
| + }() |
| + select { |
| + case err := <-done: |
| + // Refused by the open itself (O_DIRECTORY), not by the look through |
| + // a descriptor the open handed back: a device in the directory's |
| + // place would otherwise have had its driver's open run. |
| + var pe *os.PathError |
| + if !errors.Is(err, unix.ENOTDIR) || !errors.As(err, &pe) || pe.Op != "open" { |
| + t.Fatalf("a FIFO where the directory was: %v; want the open refused with ENOTDIR", err) |
| + } |
| + case <-time.After(5 * time.Second): |
| + t.Fatal("the look at a FIFO did not return: the open blocked") |
| + } |
| + // A directory still answers, by the target's own spelling. |
| + dir := filepath.Join(root, "u") |
| + writeFile(t, filepath.Join(dir, "live.sqlite")) |
| + k, err := statTarget(dir, "./live.sqlite") |
| + if err != nil || !k.regular || k.dir { |
| + t.Fatalf("a file through its directory: %+v %v", k, err) |
| + } |
| +} |
diff --git a/internal/ops/litestream_owners.go b/internal/ops/litestream_owners.go| index 597dc357c..5832e07cc 100644 |
| --- a/internal/ops/litestream_owners.go |
| +++ b/internal/ops/litestream_owners.go |
| @@ -686,7 +686,8 @@ func strictTreeDBs(root string) (dbs, skipped, problems []string) { |
| } |
| // The entry names the target as read, and the target is |
| // classified by that very spelling (from the link's own |
| - // directory when relative, through a descriptor for it: |
| + // directory when relative, through a descriptor opened by |
| + // that directory's path, as a directory and nothing else: |
| // statTarget), never by reading the link again: what the |
| // entry says and what was looked at are one string, whatever |
| // the link is re-pointed to meanwhile, and the kernel |
diff --git a/internal/ops/litestream_owners_test.go b/internal/ops/litestream_owners_test.go| index be5a4495f..64da38df8 100644 |
| --- a/internal/ops/litestream_owners_test.go |
| +++ b/internal/ops/litestream_owners_test.go |
| @@ -752,19 +752,27 @@ func TestStrictTreeNamesALinkToAFile(t *testing.T) { |
| // A target whose spelling, concatenated to the link's directory, would |
| // outgrow the kernel's path limit, though the link itself resolves: |
| // the target is looked at by itself, against the directory, and the |
| - // link is named as the file link it is. (Review sixteen, Astra 5.) |
| + // link is named as the file link it is. (Review sixteen, Astra 5.) The |
| + // target is sized from the directory the temp root gives this run, so |
| + // the concatenation outgrows the limit under any TMPDIR while the |
| + // target alone stays under it (review seventeen, Opus P2-2: a fixed |
| + // size passed only under a long temp root). |
| deep := platformDir(t) |
| for i := 0; i < 5; i++ { |
| deep = filepath.Join(deep, strings.Repeat("d", 200)) |
| } |
| mkdirs(t, deep) |
| long := writeFile(t, filepath.Join(deep, "live.sqlite")) |
| - dest := strings.Repeat("./", 1500) + "live.sqlite" |
| + n := (4097-len(deep))/2 + 1 |
| + if n < 1 { |
| + n = 1 |
| + } |
| + dest := strings.Repeat("./", n) + "live.sqlite" |
| if err := os.Symlink(dest, filepath.Join(deep, "current")); err != nil { |
| t.Fatal(err) |
| } |
| - if len(deep)+1+len(dest) <= 4096 { |
| - t.Fatalf("fixture: the concatenation is only %d bytes", len(deep)+1+len(dest)) |
| + if len(deep)+1+len(dest) <= 4096 || len(dest) >= 4095 { |
| + t.Fatalf("fixture: the concatenation is %d bytes and the target %d", len(deep)+1+len(dest), len(dest)) |
| } |
| dbs, skipped, problems = strictTreeDBs(deep) |
| if len(problems) != 0 || !contains(skipped, linkEntry(filepath.Join(deep, "current"), dest)) { |