amadan
The replicator's fifteenth build: the fourteenth review's findings
Every entry in the inventory's rendering is quoted, so a name with a line break cannot read as two entries in the drift comparison (both reviewers); a link is classified by the spelling its entry names, never by reading the link again; the Accept test tells as-written from cleaned with a "./" the cleaning would fold; the fixture makes a target with a line break. Comments both listed are corrected. Dispositions "Review fourteen" and the reports.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
7 files changed,
+399
−33
docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review14.md+201 −0docs/superpowers/specs/DISPOSITIONS-metered-replicator.md+19 −4docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review14.md+113 −0internal/ops/litestream_metered.go+17 −7internal/ops/litestream_metered_test.go+22 −8internal/ops/litestream_owners.go+16 −11internal/ops/litestream_owners_test.go+11 −3
CI passed — run details
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review14.md b/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review14.md| new file mode 100644 |
| index 000000000..a1b4a82d1 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review14.md |
| @@ -0,0 +1,201 @@ |
| +| Review-thirteen finding | Closed at `ec55b639`? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Opus P3-1 — file/link encoding collision** | **Partly.** Individual link entries are now distinct from absolute filenames. Plain filenames still collide in the serialized inventory; finding 5. | **No** for restoring the previous unprefixed encoding: the impostor-file fixture distinguishes it. **Yes** for the remaining newline collision. | |
| +| **Opus P3-2 — duplicate check uses old facts** | **Closed by inspection.** The final duplicate check uses the identities retained after ownership checks. | **Yes.** No test forces the intervening rename; removing this check remains undetected. | |
| +| **Opus P3-3 — target and classification read separately** | **Partly.** A target left changed is detected. A change followed by restoration still passes; the second read also silently drops unexpected errors. | **Yes.** Neither interleaving nor second-read errors have a distinguishing test. | |
| +| **Opus P3-4 — refusal asserted by error class alone** | **Closed.** The test now checks which entry the refusal names. | Reverting these assertions would still pass the fixture. Separately, changing its target to relative has weakened its cleaning regression coverage; finding 12. | |
| +| **Opus claims** | The obsolete function reference, coherent-foreign-tree description, link-following wording, spelled-pair qualification and Close-order comment are corrected. Encoding and classification claims remain overstated below. | Prose corrections have no behavioral test. | |
| +| **Astra 1 — Litestream pathname opens** | **Open.** | Existing tests do not force substitution during the actual opens. | |
| +| **Astra 2 — replaceable ancestry** | **Open.** | Endpoint checks pass without protecting higher ancestry. | |
| +| **Astra 3 — delayed admission escapes holds** | **Open.** | No test delays an admitted attempt across midnight beyond the margin. | |
| +| **Astra 4 — process exit loses uncertainty** | **Open.** | No durable handover/recovery test closes it. | |
| +| **Astra 5 — acceptance and inventory encoding** | **Partly.** File/link impersonation is fixed; inventory serialization remains ambiguous. | The impostor fixture fails without the encoding fix. The newline collision remains untested, and whole-entry cleaning now survives the changed acceptance fixture. | |
| +| **Astra 6 — device-open effects** | **Open.** | Final-component tests do not exercise ancestor substitution. | |
| +| **Astra 7 — hibernating aliases and skipped-file claims** | **Open.** | No test requires those competing claims to be detected. | |
| +| **Astra 8 — required coverage disappears** | **Open.** | Missing configured console databases and external-inventory reconciliation remain untested. | |
| +| **Astra 9 — tenant problems refuse the whole fleet** | **Open**, explicitly acknowledged. | Tests expect refusal and broad holds; they do not establish tenant isolation. | |
| +| **Astra 10 — route identity not bound to filesystem authority** | **Open.** | The coherent-other-owner fixture explicitly expects acceptance. | |
| +| **Astra 11 — weak tests** | **Partly.** Refusal diagnostics improved. Shutdown, watch, correspondence, interleaving and environment gaps remain. | **Yes** for the surviving mutations described in finding 12. | |
| +| **Astra claims** | Most named prose errors are corrected. Treating raw-path serialization as merely display remains incorrect; the replacement newline-fixture explanation is also unsupported. | Author-reported runs were not independently executed. | |
| + |
| +All locations below refer to **`ec55b639`**. This review used committed source and the requested diffs. No files were changed, tests run, or network commands issued. Test counterfactuals are source-based. |
| + |
| +1. **P1 — Litestream still opens outside the ownership checks.** |
| + |
| + **Locations:** `internal/lease/meteredstore.go:127`; `internal/ops/litestream_owners.go:382`; `third_party/litestream/db.go:1004`, `:1014`, `:1032`, `:1866`. |
| + |
| + A tenant passes inventory, substitutes another tenant’s database during Litestream initialization, then restores the original pathname. Litestream can retain the substituted handles while subsequent inventories—including Close’s final look—see the original inode. SQLite initialization can modify the substituted database. Later WAL opens remain independently exposed. |
| + |
| + **Smallest acceptable fix:** constrain the actual database, SQLite and sidecar opens through pinned directory authority, including subsequent opens. Checking identity after an unrestricted open cannot undo its effects. |
| + |
| +2. **P1 — Protected endpoints still permit tenant-replaceable ancestry.** |
| + |
| + **Locations:** `internal/ops/litestream_owners.go:512`, `:583`, `:810`, `:838`; `internal/ops/litestream_metered.go:960`. |
| + |
| + T owns `/srv/T`; root owns `/srv/T/bridge`; A owns the directory below `bridge`. A platform-made link targeting A’s directory passes the immediate-parent checks, but T can replace `bridge` through `/srv/T`. The same construction applies above `dataDir`, shadow state, or the directory containing an approved platform link. |
| + |
| + An approved link whose target’s **immediate parent** is tenant-owned is refused. The unchecked authority lies higher. |
| + |
| + **Smallest acceptable fix:** establish and pin trusted ancestry for claim roots, approved platform links and shadow state. |
| + |
| +3. **P1 — A delayed admitted attempt can fall outside every inherited hold.** |
| + |
| + **Locations:** `internal/ops/litestream_metered.go:238`, `:323`, `:762`; `internal/lease/requestgate.go:58`. |
| + |
| + A concrete UTC sequence remains possible: |
| + |
| + - A admits an attempt on October 2 at **23:58:00**, then pauses before recording. |
| + - Ownership changes at **23:58:10**. A detects it and holds through **23:59:10**. |
| + - B replaces A but closes unopened while A remains live. |
| + - A’s admitted attempt resumes, records and completes at **00:02 on October 3**. |
| + - A closes. Already stale, it skips the final checks and does not extend its window. |
| + - C replaces B. B and C correctly inherit A’s window, but **October 3 is absent from every ownership hold**. |
| + |
| + The lower bound and chain inheritance preserve the window supplied to them. They cannot repair its missing upper bound. |
| + |
| + **Smallest acceptable fix:** track admitted attempts through recording/completion and include their doubtful dates before finalizing handover. Increasing the fixed margin is insufficient. |
| + |
| +4. **P1 — Uncertainty still disappears across process exit.** |
| + |
| + **Locations:** `internal/ops/litestream_metered.go:344`, `:377`, `:807`. |
| + |
| + An attempt passes admission, ownership changes, and the attempt completes before invalidation. The process exits; its successor observes the new owner without a predecessor or durable uncertainty covering that completed attempt. |
| + |
| + Routine restarts can also discard inheritance awaiting delivery to replacement recorders. An unfinished ledger row covers a different case. |
| + |
| + **Smallest acceptable fix:** persist ownership/admission uncertainty before affected requests escape, and recover it before establishing successor coverage. |
| + |
| +5. **P2 — Plain filenames still collide in the inventory comparison.** |
| + |
| + **Locations:** `internal/ops/litestream_metered.go:662`, `:669`, `:677`; `internal/ops/litestream_owners.go:711`, `:722`. |
| + |
| + Consider one skipped file whose pathname is: |
| + |
| + ```text |
| + /srv/u/a.sqlite\n/srv/u/b.sqlite |
| + ``` |
| + |
| + Here `\n` denotes an actual newline in the directory name `a.sqlite\n`; beneath that directory are nested `srv/u/b.sqlite` components. |
| + |
| + Its serialized skipped section is identical to that of two separate skipped files `/srv/u/a.sqlite` and `/srv/u/b.sqlite`. Replacing the first hierarchy with those two files leaves owned identities unchanged and can leave `render()` unchanged. The running fleet therefore reports no inventory drift. |
| + |
| + The new link encoding does fix individual file/link acceptance collisions. A rebuild would also distinguish these plain filenames as separate acceptance keys. **The remaining defect is nevertheless program behavior, not merely display.** |
| + |
| + **Smallest acceptable fix:** use an unambiguous serialization for inventory comparison—such as separately encoded array elements—and quote raw paths in diagnostics. Add the newline-bearing hierarchy case. |
| + |
| +6. **P2 — Header inspection can cause device-open effects before rejecting the descriptor.** |
| + |
| + **Location:** `internal/ops/sqliteopen_unix.go:26`. |
| + |
| + The walk observes regular file `T/d/watchdog`. T replaces `d` with a link to `/dev`; the privileged open reaches `/dev/watchdog` on a suitably equipped box. The later type and identity checks cannot undo open-side effects. |
| + |
| + `O_NOFOLLOW` protects the final component. `O_NONBLOCK` addresses the FIFO wait, and `O_NOCTTY` addresses controlling-terminal acquisition. Neither confines ancestor resolution. |
| + |
| + **Smallest acceptable fix:** constrain traversal through pinned directory descriptors before opening the target, with safe file-type handling. Preserve refusal on unsupported platforms. |
| + |
| +7. **P2 — The second `Readlink` restores silent omission and does not make classification consistent.** |
| + |
| + **Locations:** `internal/ops/litestream_owners.go:675`, `:683`, `:687`. |
| + |
| + Two cases remain: |
| + |
| + - The first `Readlink` succeeds, but the second returns `EACCES`, `EIO` or another unexpected error. Line 687 silently drops the entry—even if `Stat` had already established a directory or an inaccessible target. The first read correctly reports unexpected errors; the second does not. |
| + - `p` names target A when first read, is changed to regular file F for `Stat`, then restored to A before the second read. Both strings equal A, so the inventory accepts F’s classification under A’s spelling. If A is now a directory, the problem remains hidden. This is the original change-and-restore scenario. |
| + |
| + An unchanged readable link is not falsely refused by the extra read. A mismatch is omitted as churn; normally that changes only inventory drift, **not every account’s hold**. Losing an already-owned path’s facts is what triggers the broad hold. |
| + |
| + **Smallest acceptable fix:** report unexpected second-read errors; reserve churn for disappearance/type replacement or an observed change. Classify the captured target through stable link/parent authority instead of resolving the mutable link pathname again. Add controlled error and change-and-restore cases. |
| + |
| +8. **P2 — Hibernating aliases and skipped-file claims still hide competing ownership.** |
| + |
| + **Locations:** `internal/ops/litestream.go:381`, `:479`, `:540`, `:709`; `internal/ops/litestream_owners.go:166`, `:241`. |
| + |
| + A live unit claims `/var/lib/carlos-app/u`, reached through its platform state link. Another account’s hibernating exec route names the same file as `/var/lib/private/carlos-app/u/main.db`. The hibernating path contributes no candidate, and textual matching misses the shared inode. |
| + |
| + Claims on `u/.cache/app.db` or `u/main.sqlite` can likewise remain only in `Skipped`, without their competing claimants. Two hibernating claims also bypass listed-file conflict checks. |
| + |
| + **Smallest acceptable fix:** reconcile claims by file identity against listed and skipped files, retaining claimant information for skipped entries. |
| + |
| +9. **P2 — Required coverage can still disappear without an entry.** |
| + |
| + **Locations:** `internal/ops/litestream.go:507`, `:525`; `internal/ops/litestream_owners.go:639`, `:660`, `:707`. |
| + |
| + Set `CONSOLE_DB` to a missing file outside the globs and route claims. It disappears like the optional console path on a box without a console. Nothing distinguishes “configured but missing” from “not required.” |
| + |
| + Empty tree-only files and entries disappearing before their first inspection remain deliberate omissions. A database known only to the external replicator, outside these trees and explicit live claims, reaches no list. |
| + |
| + **Smallest acceptable fix:** distinguish required platform databases from optional ones and reconcile against the external replicator’s inventory before cutover. |
| + |
| +10. **P2 — Ordinary tenant activity can refuse replacement replication for every account.** |
| + |
| + **Locations:** `internal/ops/litestream_owners.go:694`, `:699`; `internal/ops/litestream_metered.go:656`, `:853`, `:879`. |
| + |
| + Installing a virtual environment or packages can introduce numerous ordinary links. Regular-file links require acceptance; directory links are unconditional problems. The current fleet reports drift, but its replacement refuses. An unrelated ownership change can then leave every account waiting for that tenant’s inventory to be resolved. |
| + |
| + A legitimate concurrent rename caught by either inode check also removes an owned file’s facts and holds every recorder. On a stable layout, the new kept-facts duplicate check correctly rejects actual aliases; it does not confuse distinct inodes. |
| + |
| + **Smallest acceptable fix:** implement tenant-scoped failure and holds, or explicitly adopt the box-wide policy with an operational recovery procedure before wiring. |
| + |
| +11. **P3 — Route identity remains unbound to independently established filesystem ownership.** |
| + |
| + **Locations:** `internal/ops/litestream_owners.go:176`, `:435`, `:495`, `:559`; `internal/ops/litestream_owners_unix_test.go:381`. |
| + |
| + Route A naming B’s coherent directory and file attributes B’s file to A. The test explicitly expects this. |
| + |
| + Root-run units and tenants sharing the platform UID can similarly claim otherwise-unclaimed qualifying platform files. This is **not acceptance from anywhere**: permissions, links, filesystem, regular-file status, link count and sidecars still constrain it. |
| + |
| + A distinct tenant UID cannot use root/platform equivalence to claim a root-owned file directly inside its tenant-owned directory. The reverse—tenant-owned file directly inside a protected platform directory—is generally refused by the final owner comparison. |
| + |
| + **Smallest acceptable fix:** bind route claims to independently established roots and identities, with explicit treatment of trusted platform workloads. Deployment approval alone does not establish that invariant. |
| + |
| +12. **P3 — The changed acceptance fixture loses one regression check; other test gaps remain.** |
| + |
| + **Locations:** `internal/ops/litestream_metered_test.go:689`, `:829`, `:963`, `:1062`, `:1380`, `:1496`, `:1862`; `internal/ops/litestream_owners_test.go:689`. |
| + |
| + The new relative-target fixture compares `live.sqlite` with `x/../live.sqlite`. If whole-entry cleaning is restored, the `..` removes the component containing `current" -> "x`; it does **not** produce the accepted `current" -> "live.sqlite"` key. The test still gets its expected refusal and diagnostic. It therefore passes without the as-written acceptance rule it names. |
| + |
| + Use targets such as `x/live.sqlite` and `x/y/../live.sqlite`, with the directories present, or retain the earlier absolute-target case. Whole-entry cleaning would then collapse the two keys. |
| + |
| + Other surviving counterfactuals remain: |
| + |
| + - Removing `store.Close` can satisfy cleared-pointer and refusing-gate assertions; retained handles and worker exit are not asserted. |
| + - Removing the watch’s `lookAgain` invocation leaves direct-call file tests intact. |
| + - Swapping fleet database recorders can preserve positive counts for both databases. |
| + - Removing admission’s pre-witness under-lock stale check is not independently distinguished by the post-witness fixture. |
| + - Removing the latch’s under-lock recheck survives sequential tests; racing successors are outside the caller contract. |
| + - Removing `prepareShadowState`’s data-directory check survives the fixture refused earlier by inventory. |
| + - Removing either cross-check inode comparison, the new kept-facts duplicate check, or the second `Readlink` has no controlled distinguishing case. |
| + - Replacing proper quoting with surrounding quote characters survives the current ordinary-name fixtures. |
| + |
| + The 300 ms negative Close check can pass before the second goroutine runs. The clean-start test reconstructs time from Unix nanoseconds, so its comparisons depend on wall-clock stability. |
| + |
| + **Smallest acceptable fix:** restore a cleaning-sensitive acceptance fixture, add independently specified encoding expectations and controlled filesystem interleavings, and assert shutdown/watch/recorder behavior directly. |
| + |
| +The other requested checks hold with these limits: |
| + |
| +| Area | Source-based result | |
| +|---|---| |
| +| **Tenant-file chain, starting directory and platform links** | `litestream_owners.go:464`, `:495`, `:500`, `:509`, `:512` reject foreign starting directories, tenant links and foreign stopping parents on stable paths. Every accepting `tenantFile` path reaches the top check. A final-component `DBPath` link is refused at `:435`. Higher replaceable ancestry remains finding 2. | |
| +| **All claimant checks and platform layouts** | `claimChecks` at `litestream_owners.go:278` retains every distinct root/kind, including two trees, tree-plus-file and unit-plus-exec claims. Ordinary DynamicUser links, protected operator-chosen paths and isolated-instance directories can pass. No ranking discards a restriction. | |
| +| **One-inode rule** | `litestream_owners.go:193` retains the first successful check’s facts and requires later checks to agree. `:217` extends duplicate rejection to those retained facts. Order does not change the answer on a stable layout; concurrent changes can cause conservative refusal. Neither rule establishes an atomic filesystem snapshot. | |
| +| **Admission and retries** | Checks at `litestream_metered.go:212`, `:224`, `:238` cover staleness before locking and during the witness read. A change after the last load remains the admitted gap. `witnessUnchanged` makes at most three reads, accepts recovered errors, stops on a witnessed change and treats exhausted errors as uncertainty. Context deadlines do not bound mutex waiting or filesystem `Stat`. | |
| +| **Registry generation** | `registry.go:569`, `:602`, `:612` cover the six consumed columns, insert/delete and rowid immutability. Older-build raw writes still fire the triggers. Same-value updates, unrelated columns and Litestream bookkeeping do not move it. Literal “nothing else” is too strong: database-less route insertion/deletion and semantically irrelevant changes to those six columns also move it. | |
| +| **Replacement chain and Close** | `chainClosed` checks a latched predecessor’s own closure first. Window publication, detachment and latch publication share `staleMu`; published chain reads take that lock. Within the single-chain contract, replacement cannot open beside an open/draining predecessor, and an unopened successor cannot latch through one. Intermediate fleets receive inheritance once. Reading a closed stale fleet’s window is intentional. | |
| +| **Final file look** | Order is store stopped → watch stopped → file look → witness → inheritance → closed (`litestream_metered.go:736`, `:739`, `:763`, `:766`, `:772`, `:773`). A persistent changed/missing owned file on a current fleet holds from the last clean look. Already-stale fleets skip the checks. A harmless change after replication stops can over-hold everyone; the inventory walk can block Close. | |
| +| **Lineage** | `ownedReplicaKey` at `litestream_metered.go:938` combines account and cleaned path. Different valid accounts do not share lineage or shadow keys, including after renaming. A→B→A at one path resumes A’s old lineage; changing app within an account does not create a new key. External-lineage separation remains unenforced. | |
| +| **Strict inventory** | Database extensions are recognized case-insensitively, although uppercase `.DB` is skipped rather than streamed by tree discovery. Every nonempty regular file not recognized by name gets a header read, including sidecar-shaped names; `sidecarOf` is gone. Missing/relative unit claims, units without `DBPath`, empty explicit claims and discovered files failing their final look are reported. Explicit exec claims receive ownership checks and do not change external config generation. | |
| +| **Links and traversal** | An unchanged encoded pair remains accepted; relative targets retain literal `Readlink` spelling. A link reaching a file through a directory link is classified by the final target. Leading `..` permits systemd’s usual state-link shape; ascents after named components are refused. Stable accepted traversal agrees with kernel resolution, subject to the ancestry/race gaps above. | |
| +| **Fixtures and root CI** | Relevant fixture modes are explicitly chmodded, so umask `077` does not narrow them. `platformDir` resolves a linked temporary root. Basename-based fake ownership and fixed fake UIDs remain assumptions. Root skips the mixed-root/platform test, unreadable-directory cases and sealed-target case. `.amadan/ci.d/10-gate:52`, `Makefile:18` and `.amadan/README.md:73` do **not** establish CI’s UID. | |
| +| **Open-item scope and reports** | The pathname-open gap includes retained handles, reopens and sidecars; the exit gap includes routine restarts. Same-filesystem bind mounts remain undetected when only one alias is inventoried; two listed aliases are rejected by identity. No external-inventory comparison exists. Both reviewers’ review-eight, review-nine and review-thirteen reports are present. | |
| + |
| +**Claims the code contradicts** |
| + |
| +- **Dispositions `:283`: plain-name ambiguity is “display.”** `render()` is compared at `litestream_metered.go:662`. Its ambiguity can suppress inventory drift, as finding 5 demonstrates. |
| +- **Dispositions `:285`: the link classification race is fixed.** A second equal target string does not exclude change-and-restoration around `Stat`. Unexpected second-read errors are also silently omitted. |
| +- **Dispositions `:271`: every non-ENOENT `Readlink` error is a problem.** The first read now deliberately exempts EINVAL; the second drops every error. |
| +- **Dispositions `:268`: the acceptance test fails with whole-entry cleaning.** That describes earlier mutation evidence, not the current relative-target fixture. |
| +- **`litestream_owners_test.go:690`: a temporary root cannot hold the newline fixture.** Nothing in `platformDir`, `mkdirs` or the Unix filename rules shown here establishes that restriction. A newline-bearing child name can be constructed independently of the temporary root’s generated name. |
| +- **Unconditional final-witness descriptions remain in `Replaces`, `inheritHold` and `Open`** (`litestream_metered.go:102`, `:476`, `:550`). A fleet current on entering Close can become stale during its final file look and skip the witness; Close’s own comment correctly states that case. |
| +- Author-reported passing tests and mutation runs are neither confirmed nor disproved by this source-only review. |
| + |
| +**Verdict: reject.** |
| \ No newline at end of file |
diff --git a/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md b/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md| index cd592bb7c..03ce0774e 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md |
| @@ -265,10 +265,10 @@ Reports: `OPUS-REVIEW-metered-replicator-review12.md`, `CODEX-FINDINGS-metered-r |
| | Finding | Disposition | Where | Reproduction | |
| | --- | --- | --- | --- | |
| -| Opus P3-1, Astra 5 (P2): the link entry was not a safe acceptance key or encoding. | Fixed: a link entry is `path -> "target"` with the target quoted (`strconv.Quote`: separators, line breaks and `..` read as what they are), and `Accept` compares a link entry as written, never cleaned; a plain path is still accepted as cleaned. [Review thirteen: a regular file whose own name spelt the pair read as the accepted link entry; the fourteenth build quotes both halves behind the word `link`, which no absolute path begins with, so the encoding is one to one.] Relative targets are rendered as `Readlink` gives them (the classification follows the kernel's resolution, the entry names the spelling). | `litestream_owners.go` `linkEntry`, `isLinkEntry`; `litestream_metered.go` `MeteredFleetStore` | `TestAcceptComparesALinkEntryAsWritten` (the pair accepted builds the store; the link re-pointed to the same file through `x/..` refuses it until the new pair is accepted); `TestStrictTreeNamesALinkToAFile` (a target spelt through `..` is named as written); each fails with the target cleaned, or with link entries cleaned by `Accept` (mutation runs) | |
| +| Opus P3-1, Astra 5 (P2): the link entry was not a safe acceptance key or encoding. | Fixed in the thirteenth build as `path -> "target"` with the target quoted, `Accept` comparing a link entry as written and a plain path as cleaned [the fourteenth build made it `link "path" -> "target"`, both halves quoted; the fifteenth build's Accept fixture (`sub/live.sqlite` against `sub/./live.sqlite`) is the one that tells as-written from cleaned, the fourteenth's `x/..` case did not]. [Review thirteen: a regular file whose own name spelt the pair read as the accepted link entry; the fourteenth build quotes both halves behind the word `link`, which no absolute path begins with, so the encoding is one to one.] Relative targets are rendered as `Readlink` gives them (the classification follows the kernel's resolution, the entry names the spelling). | `litestream_owners.go` `linkEntry`, `isLinkEntry`; `litestream_metered.go` `MeteredFleetStore` | `TestAcceptComparesALinkEntryAsWritten` (the pair accepted builds the store; the link re-pointed to the same file through `x/..` refuses it until the new pair is accepted); `TestStrictTreeNamesALinkToAFile` (a target spelt through `..` is named as written); each fails with the target cleaned, or with link entries cleaned by `Accept` (mutation runs) | |
| | Opus P3-2: the checks on one file were not required to see one inode. | Fixed: the facts kept are the first check's, and a later check whose `Dev`/`Ino` differ refuses the file ("changed under the look"). No deterministic test: the case is a rename between two checks of one inventory; stated. | `litestream_owners.go` `fleetInventory` | | |
| | Opus P3-3, Astra 11: the every-claim test could not tell "every check" from "the tree check only", and its comment called the needed layout impossible; the root/platform test is vacuous as root; the sealed-link case is skipped as root. | Fixed: the layout exists (the tree's user's file in another user's directory inside the tree: the tree check accepts, the file check refuses the directory) and is the test's second case; the root/platform test skips as root with a message. The sealed case stays skipped as root (root can look at anything), stated. | tests | `TestEveryClaimOnAFileMustPass`; fails with tree checks alone whenever a tree claim exists (mutation run) | |
| -| Opus P3-4: a `Readlink` error was dropped. | Fixed: ENOENT is churn; any other error is a problem. | `litestream_owners.go` `strictTreeDBs` | no deterministic test (a link that cannot be read by this process while its directory can be listed is not arranged); stated | |
| +| Opus P3-4: a `Readlink` error was dropped. | Fixed: ENOENT and EINVAL (no longer a link) are churn; any other error is a problem. | `litestream_owners.go` `strictTreeDBs` | no deterministic test (a link that cannot be read by this process while its directory can be listed is not arranged); stated | |
| | Astra 6 to 9 (P2s), 1 to 4 (P1s), 10: the device open; hibernating aliases and claims on skipped files; required coverage that can disappear (`CONSOLE_DB` naming a missing database; external inventory); the box-wide refusal; litestream's opens by path; ancestry; delayed admission; process exit; the uid binding. | Open, as before; each stated where it applies; the box-wide refusal and the uid binding are decisions before wiring. | | | |
| | Claims (both): the old ranking paragraph left above the new one; `strictestClaim` named in `litestream.go`; "the two things"; "a link re-pointed later reads as a change" (now true at `Accept` too); "the facts are one inode's" (now checked); the kinds' docs; the test comments ("no layout", "another user's directory refused", "the right recorder", "an accepted link cannot be turned unseen"); the witness-once-more sentences at `inheritHold`, `Open` and `Close`; this file's rows above. | Corrected in place. | | | |
| @@ -280,11 +280,26 @@ Reports: `OPUS-REVIEW-metered-replicator-review13.md`, `CODEX-FINDINGS-metered-r |
| | Finding | Disposition | Where | Reproduction | |
| | --- | --- | --- | --- | |
| -| Opus P3-1, Astra 5 (P2): the link entry was not injective. | Fixed: a skipped link is `link "path" -> "target"`, both halves quoted behind a word no absolute path begins with, so no file name and no other pair renders the same; `Accept` compares such an entry as written. A plain file's name with a line break can still render as two lines in `render()` and in an error text; that is display, not acceptance (a plain entry is compared cleaned, as one string), and is stated. A problem names its path first and its target quoted (nothing compares a problem with `Accept`). | `litestream_owners.go` `linkEntry`, `isLinkEntry` | `TestAcceptComparesALinkEntryAsWritten` (a relative target as tenant software makes them; the pair accepted builds the store; re-pointed through `x/..` it is refused naming the new pair; a database file named like the pair is refused as the plain file it is); fails with the unprefixed encoding (mutation run) | |
| +| Opus P3-1, Astra 5 (P2): the link entry was not injective. | Fixed: a skipped link is `link "path" -> "target"`, both halves quoted behind a word no absolute path begins with, so no file name and no other pair renders the same; `Accept` compares such an entry as written. A plain file's name with a line break rendered as two lines in `render()`, which is the drift comparison, not display [review fourteen, both reviewers]; the fifteenth build quotes every entry in `render()`. A problem names its path first and its target quoted (nothing compares a problem with `Accept`). | `litestream_owners.go` `linkEntry`, `isLinkEntry` | `TestAcceptComparesALinkEntryAsWritten` (a relative target as tenant software makes them; the pair accepted builds the store; re-pointed through `x/..` it is refused naming the new pair; a database file named like the pair is refused as the plain file it is); fails with the unprefixed encoding (mutation run) | |
| | Opus P3-2: the one-inode rule did not reach the listing-time duplicate check. | Fixed: after the checks, two owned paths whose kept facts are one file are both unowned ("became this same file under the look"). No deterministic test (a rename between two files' checks within one inventory); stated. | `litestream_owners.go` `fleetInventory` | | |
| -| Opus P3-3: the link's target was read, then the link classified by a second read. | Fixed: the target is read again after the classification, and a change between the two is churn; a link that is no longer a link (EINVAL) is churn. No deterministic test; stated. | `litestream_owners.go` `strictTreeDBs` | | |
| +| Opus P3-3: the link's target was read, then the link classified by a second read. | Fixed in the fourteenth build by a second read, which review fourteen showed does not close a re-point-and-restore around the `Stat`; the fifteenth build classifies the spelling the entry names (`Stat` of the target as read, from the link's directory when relative), so the entry and what was looked at are one string whatever the link is re-pointed to meanwhile; a link that is no longer a link (EINVAL) is churn. No deterministic test; stated. | `litestream_owners.go` `strictTreeDBs` | | |
| | Opus P3-4: the `Accept` test asserted refusal by class alone. | Fixed: it asserts the entry named in the refusal, and that the accepted one is not. | tests | the same test | |
| | Astra 1 to 4, 6 to 11: the deep items, the hibernating aliases and skipped-file claims, required coverage that can vanish, the box-wide refusal, the uid binding, the remaining test gaps. | Open, as before, stated where each applies; the box-wide refusal and the uid binding are decisions before wiring. | | | |
| | Claims (both): the review-ten row's `strictestClaim` and "refused when named directly in another user's directory"; the review-twelve row's residue; "a link is never followed" in two test comments; the "line break" claim in a fixture comment; the close-order test comment; "no target can forge"; "the pair that is there". | Corrected in place. | | | |
| Package tests pass under `/tmp` and `~/.cache` (the author's runs, not as root). The full gate on this tip is next; the fourteenth build has not been read by a reviewer. |
| + |
| +## Review fourteen (reviewed `ec55b639`; Opus accepted with fixes, Astra rejected) |
| + |
| +Reports: `OPUS-REVIEW-metered-replicator-review14.md`, `CODEX-FINDINGS-metered-replicator-review14.md` (in the tree from the fifteenth build's commit). Both found the thirteenth round's link-entry collision closed (a file name no longer reads as an accepted link entry) and both found what the fourteenth build left: a plain name with a line break rendered as two entries in `render()`, which is the drift comparison and not display as the dispositions had said; the second `Readlink` did not close a re-point-and-restore around the `Stat`, and dropped a second-read error; the Accept test's `x/..` case no longer told as-written from cleaned. The fifteenth build: |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Opus P3-2, Astra 5 (P2): `render()` wrote names raw, so a name with a line break read as two entries and two as one, and the running fleet's drift comparison could be defeated. | Fixed: every entry in `render()` is quoted (`strconv.Quote`), owned paths included. | `litestream_metered.go` `render` | `TestRenderQuotesEveryEntry`; fails with entries written raw (mutation run) | |
| +| Opus P3-1, Astra 7 (P2): the second `Readlink` missed a link re-pointed and put back, and dropped a second-read error. | Fixed: the second read is gone; the target is classified by the spelling the entry names (a `Stat` of the target as read, from the link's directory when relative), so the entry and what was looked at are one string whatever the link points to meanwhile. What that spelling resolves to is not pinned, as stated before. | `litestream_owners.go` `strictTreeDBs` | no deterministic test (a tenant-timed re-point around one `Stat`); stated | |
| +| Opus P3-3, Astra 12: the Accept test no longer told as-written from cleaned. | Fixed: the fixture accepts `sub/live.sqlite` and re-points to `sub/./live.sqlite`, which cleaning folds to the accepted entry. | tests | `TestAcceptComparesALinkEntryAsWritten`; fails with link entries cleaned by `Accept` (mutation run) | |
| +| Opus claims: the "passes over on purpose" list; the newline-fixture sentence ("a temp root cannot hold it": false). | Corrected: the list names a link that is something else when read; the fixture makes a target with a line break and asserts it quoted. | `litestream_owners.go`, `litestream_owners_test.go` | `TestStrictTreeNamesALinkToAFile` | |
| +| Astra 1 to 4, 6, 8 to 11: the deep items, the hibernating aliases and skipped-file claims, required coverage that can vanish, the box-wide refusal, the uid binding, the remaining test gaps (shutdown, watch, attribution, interleavings, root). | Open, as before, stated where each applies; the box-wide refusal and the uid binding are decisions before wiring. | | | |
| +| Claims (both): this file's rows for the thirteenth-build link entry, the "display" sentence, the Readlink race "fixed", the EINVAL exemption; the unconditional witness sentences at `Replaces`, `inheritHold` and `Open` (a fleet made stale by the final look skips the witness). | Corrected in place; the three witness sentences now say "unless already stale, or made stale by the final look". | this file, `litestream_metered.go` | | |
| + |
| +Package tests pass under `/tmp` and `~/.cache` (the author's runs, not as root). The full gate on this tip is next; the fifteenth build has not been read by a reviewer. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review14.md b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review14.md| new file mode 100644 |
| index 000000000..12dd6d8a9 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review14.md |
| @@ -0,0 +1,113 @@ |
| +# Fourteenth review: feat/carloku-metered-replicator-20261001 at `ec55b639` |
| + |
| +**Verdict: accept with fixes.** All four review-thirteen Opus findings are addressed, and so is Astra 5's file/link impersonation. Two of those fixes are only partly closed: the second `Readlink` does not catch a link that is re-pointed and then put back, and the `Accept` test no longer checks the as-written comparison it is named for. The plain-name line-break residue is described in the dispositions as "display only". It is not: `render()` is the running fleet's drift comparator. |
| + |
| +The fourteenth build touches only: |
| +- the link encoding (`linkEntry`, `isLinkEntry`), |
| +- the link classification in `strictTreeDBs`, |
| +- the owned-inode duplicate check in `fleetInventory`, |
| +- comments, tests and dispositions. |
| + |
| +The registry, admission, Close, the latch and lineage are untouched. So A–X stand as review thirteen answered them, and Astra 1–4, 6–11 are unchanged and open. I read the source only with `git show ec55b639:` and ran nothing. |
| + |
| +## Review thirteen, finding by finding |
| + |
| +| Finding | At `ec55b639` | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Opus P3-1 / Astra 5 (first half):** a file named like the pair reads as the accepted link entry | **Closed.** `link "p" -> "t"` (`litestream_owners.go:883-890`). `strconv.Quote` round-trips, and every plain skipped entry is absolute, so no plain entry can begin with `link "`. Every source of plain skipped entries is absolute: tree roots and claims are required absolute (`litestream.go:367`, `:392`), and `dataDir` is absolute (`litestream_metered.go:828`). | **No.** The impostor case (`litestream_metered_test.go:1906`) builds the store under the unprefixed encoding. | |
| +| **Astra 5 (second half):** a line break in a plain name | **Open, and mis-stated** (P3-2 below). | No test. | |
| +| **Opus P3-2:** the one-inode rule stops at the file | **Closed** (`litestream_owners.go:212-234`). | **Yes** (no test, as stated). | |
| +| **Opus P3-3:** two non-atomic reads of the link | **Partly closed.** A→B is caught. A→B→A is not (P3-1 below). | **Yes** (no test, as stated). | |
| +| **Opus P3-4:** refusal asserted by class only | **Closed** as an assertion. The re-pointed case no longer tells "as written" from "cleaned" (P3-3 below). | n/a | |
| +| **Opus claims** | Corrected, except as listed under Claims. | n/a | |
| +| **Astra 1–4, 6–11** | **Open**, code unchanged. Astra 11's surviving mutations still survive, plus the two added here. | n/a | |
| + |
| +## Findings |
| + |
| +### P3-1. The second `Readlink` misses a link re-pointed and put back (Z, K) |
| + |
| +**Where:** `litestream_owners.go:675` (`Readlink`), `:683` (`Stat(p)`), `:687` (`Readlink` again). |
| + |
| +**Scenario:** |
| +1. `link "/S/current" -> "A"` was accepted while A was a regular file. A is now a directory, so `current` should be a problem. |
| +2. During a build, the tenant renames a prepared link to F over `current`, and then the link to A back again. Both renames are atomic and timed around the `Stat`. |
| +3. Both reads give A, the `Stat` saw F (regular), and the accepted pair passes. |
| + |
| +The disposition says the race between two reads is "fixed". Any number of reads of `p` leaves the same gap open. |
| + |
| +**Bound:** it needs a tenant-timed race at the build, and what sits behind A is the tenant's own. |
| + |
| +**Smallest fix:** classify the spelling that the entry names, not `p`. That is `os.Stat(dest)` when `dest` is absolute, or `os.Stat(filepath.Join(dir, dest))` when relative. The second `Readlink` then goes away. What the spelling resolves to stays unpinned, as already stated. |
| + |
| +**Related, same lines:** |
| +- A second-read error other than ENOENT/EINVAL is now silently treated as churn (`:687`). The first read reports such errors as a problem (Opus P3-4's fix). |
| +- EINVAL is treated as churn without a fresh `Lstat` (`:676`). A link atomically replaced by a regular file is therefore not looked at during this look. |
| + |
| +Both belong to the existing "vanished under the look" class, and the next look corrects them. But the doc comment's "passes over on purpose" list (`:639`) names neither. |
| + |
| +### P3-2. A plain name with a line break defeats the drift comparison, not just the display (Y) |
| + |
| +**Where:** `litestream_metered.go:662` (`inv.render() != f.inventory` sets drift) and `:669-680` (paths written raw, entries joined with `\n`). |
| + |
| +**Scenario:** |
| +1. The build had `/S/a.sqlite` and `/S/b.sqlite` skipped and accepted. |
| +2. The tenant removes both and creates a single file at `/S/a.sqlite\n/S/b.sqlite`. That is a directory named `a.sqlite⏎` holding the nested directories that spell `/S/`, then `b.sqlite`. |
| +3. The rendered output is byte-identical, so `Current` never returns `ErrInventoryChanged`. |
| + |
| +Only rebuilds triggered for some other reason see the new entry (the rebuild does refuse it as unaccepted). Owned paths are protected by the facts check, which runs before `render`. Unowned entries and problems carry fixed suffixes and were empty at the build, so they cannot collide. |
| + |
| +**Impact:** |
| +- A tenant can rearrange its skipped files without the running fleet noticing. |
| +- With effort, it can hide a new skipped database until the next rebuild. |
| +- Nothing is billed or replicated wrongly. |
| + |
| +**Smallest fix:** quote every path in `render()` with `strconv.Quote`. The dispositions' "that is display, not acceptance" (`DISPOSITIONS:283`) should become: "drift comparison, now quoted". |
| + |
| +### P3-3. The `Accept` test no longer tells "as written" from "cleaned" (H, V) |
| + |
| +**Where:** `litestream_metered_test.go:1895`. |
| + |
| +Suppose `Accept` and the skipped-key loop both cleaned link entries (`litestream_metered.go:863-874`): |
| +- The accepted entry `link "/S/current" -> "live.sqlite"` is left unchanged by cleaning. |
| +- In the re-pointed entry `link "/S/current" -> "x/../live.sqlite"`, the `..` pops the component `current" -> "x`, so it cleans to `link "/S/live.sqlite"`. |
| +- That is not equal to the accepted entry, so the build is still refused, and the error still names `repointed` (`unseen` keeps the raw entry). |
| + |
| +So the test passes without the as-written rule. |
| + |
| +The rule is still load-bearing: |
| +- `-> "sub/./f"` and `-> "sub/f"` clean to the same entry. |
| +- `link "/S/a" -> "x/../f"` and `link "/S/b" -> "y/../f"` both clean to `link "/S/f"`. |
| + |
| +**Smallest fix:** accept `-> "sub/live.sqlite"`, then re-point to `"sub/./live.sqlite"` or `"sub//live.sqlite"`. Under cleaning that folds to the accepted entry and the build would succeed, so the test can tell the two apart. |
| + |
| +## The new lettered questions |
| + |
| +**Y.** The link entry is one to one against every name a tenant can make: |
| +- `Quote` is injective. |
| +- The prefix cannot begin an absolute path. |
| +- Every source of plain skipped entries is absolute. |
| +- A cleaned plain entry can equal a link entry only through operator input such as `./link "…"`, never through a tenant's name. That is harmless. |
| +- `render()` with a line break: P3-2. The `unseen` error text (`, `-joined) and the raw `p` in problem texts are display only and fail closed. |
| + |
| +**Z.** Neither new check refuses a stable layout: |
| +- **Duplicate check:** two stable names of one inode are already caught as "twice" at listing time. Hard links are refused by the link count, and bind mounts share the device and inode, so they are caught by "twice" too. |
| +- **Second `Readlink`:** gives the same answer on a stable link. |
| + |
| +Cost at a running look: |
| +- The duplicate check is O(n) over owned paths and adds no new hold. Any rename that makes it fire also changes the kept facts of the renamed-over path against the fleet's, and that already goes stale and holds every account. At the build, the refusal is transient. |
| +- The second `Readlink` is one extra syscall per link per look. A link that tenant software re-points often (a deploy's `current`) now appears in and drops out of `Skipped` between looks. That raises drift (`ErrInventoryChanged`, a rebuild request), not a hold. |
| + |
| +**A–X:** as review thirteen found. Nothing they read changed apart from the lines above. CI's user is still not established by committed source (X). |
| + |
| +## Claims the code contradicts |
| + |
| +- **`DISPOSITIONS:283`:** "that is display, not acceptance". `render()` is the drift key (P3-2). |
| +- **`DISPOSITIONS:283`:** the Opus P3-3 row says "fixed". It fixes only a link re-pointed and left re-pointed (P3-1). |
| +- **`DISPOSITIONS:268`:** |
| + - It still opens with "a link entry is `path -> "target"`". |
| + - Its Reproduction column says the `Accept` test "fails … with link entries cleaned by `Accept`". With the fourteenth build's relative targets it no longer does (P3-3). |
| +- **`litestream_owners_test.go` ≈`:686-689`:** "a fixture cannot make a link to a name a temp root refuses to hold". Linux tmpfs and ext4 hold names with line breaks, so a regular file `a⏎b` and a link to it can be made. |
| +- **`litestream_owners.go:639-650`:** the "passes over on purpose" list leaves out a link re-pointed between reads, a link replaced by a non-link (EINVAL), and a second-read error. |
| +- **Dispositions, "Package tests pass … not as root":** this is the author's report. Not verified here. |
| + |
| +**Verdict: accept with fixes.** Fix P3-1 to P3-3 and the claims. Astra 1–4 and 8, approval H's uid binding, and the box-wide refusal decision still block wiring. |
diff --git a/internal/ops/litestream_metered.go b/internal/ops/litestream_metered.go| index b6927ba14..70e50c38a 100644 |
| --- a/internal/ops/litestream_metered.go |
| +++ b/internal/ops/litestream_metered.go |
| @@ -7,6 +7,7 @@ import ( |
| "os" |
| "path/filepath" |
| "sort" |
| + "strconv" |
| "strings" |
| "sync" |
| "sync/atomic" |
| @@ -100,7 +101,8 @@ type MeteredFleetOptions struct { |
| // Replaces is the fleet this one is built to take over from, if any. |
| // The old fleet must be closed before this one is opened, and closing |
| // it looks at the files and asks the witness one last time each (a |
| - // fleet already stale skips both). At Open, if that fleet went |
| + // fleet already stale skips both; one made stale by the look skips |
| + // the witness). At Open, if that fleet went |
| // stale, every recorder of this one is told the same days are |
| // incomplete: the new owner of a database that changed hands is one |
| // of them, and the old fleet never had its recorder. The caller keeps |
| @@ -474,8 +476,9 @@ func (f *MeteredFleet) latch() (from, at time.Time, stale, first bool) { |
| // tells this fleet's recorders, and keeps it to pass on. It is called at |
| // Open and at Close, and takes effect only once every fleet in the chain |
| // has closed (closing looks at the files and asks the witness one last |
| -// time each, unless the fleet is stale already, so whatever the old fleet |
| -// had to say about its last days it has said by then): a fleet |
| +// time each, unless the fleet is stale already or made stale by the |
| +// look, so whatever the old fleet had to say about its last days it has |
| +// said by then): a fleet |
| // closed before its predecessor does not latch, and answers for its |
| // predecessor through staleWindow until a later fleet reads it. |
| func (f *MeteredFleet) inheritHold() { |
| @@ -549,8 +552,9 @@ func (f *MeteredFleet) Open(ctx context.Context) error { |
| } |
| // The fleet this one replaces has closed, and closing looks at the |
| // files and asks the witness one last time each (unless it was stale |
| - // already), so whatever it had to say about its last days it has |
| - // said by now. That is told to this fleet's recorders here, once: a |
| + // already, or the look made it so), so whatever it had to say about |
| + // its last days it has said by now. That is told to this fleet's |
| + // recorders here, once: a |
| // database that changed hands has a new owner, whose recorder the old |
| // fleet never had. Read at build it would be missed whenever the new |
| // fleet is built before the old one notices. |
| @@ -667,14 +671,20 @@ func (f *MeteredFleet) lookNow(final bool) { |
| // render is an inventory as one comparable string: what is owned and by |
| // whom, what is unowned, skipped, or could not be looked at. |
| func (inv FleetInventory) render() string { |
| + // Every entry quoted: the rendering is what the watch compares, and a |
| + // name holding a line break must not read as two entries (nor two as |
| + // one). |
| var b strings.Builder |
| for _, o := range inv.Owned { |
| - fmt.Fprintf(&b, "owned %s %s/%s %s\n", o.Path, o.Account, o.App, o.Host) |
| + fmt.Fprintf(&b, "owned %s %s/%s %s\n", strconv.Quote(o.Path), o.Account, o.App, o.Host) |
| } |
| for _, list := range [][]string{inv.Unowned, inv.Skipped, inv.Problems} { |
| sorted := append([]string(nil), list...) |
| sort.Strings(sorted) |
| - b.WriteString("--\n" + strings.Join(sorted, "\n") + "\n") |
| + b.WriteString("--\n") |
| + for _, e := range sorted { |
| + b.WriteString(strconv.Quote(e) + "\n") |
| + } |
| } |
| return b.String() |
| } |
diff --git a/internal/ops/litestream_metered_test.go b/internal/ops/litestream_metered_test.go| index 3fe138fac..7701a7448 100644 |
| --- a/internal/ops/litestream_metered_test.go |
| +++ b/internal/ops/litestream_metered_test.go |
| @@ -1866,14 +1866,14 @@ func TestAcceptComparesALinkEntryAsWritten(t *testing.T) { |
| state := filepath.Join(platformDir(t), "u") |
| mkdirs(t, state) |
| db := writeFile(t, filepath.Join(state, "main.db")) |
| - live := writeFile(t, filepath.Join(state, "live.sqlite")) |
| + live := writeFile(t, filepath.Join(state, "sub", "live.sqlite")) |
| // A relative target, as tenant software makes them. |
| - if err := os.Symlink("live.sqlite", filepath.Join(state, "current")); err != nil { |
| + if err := os.Symlink("sub/live.sqlite", filepath.Join(state, "current")); err != nil { |
| t.Fatal(err) |
| } |
| upsert(t, reg, registry.Route{Host: "u.example.com", Account: "bac", App: "one", DBPath: state, Backing: registry.BackingUnit}) |
| o := fleetOptions(reg, dir) |
| - entry := linkEntry(filepath.Join(state, "current"), "live.sqlite") |
| + entry := linkEntry(filepath.Join(state, "current"), "sub/live.sqlite") |
| if _, err := MeteredFleetStore(o); !errors.Is(err, ErrUnmeterable) || !strings.Contains(err.Error(), entry) { |
| t.Fatalf("a link to a file not accepted: %v; want the store refused naming %s", err, entry) |
| } |
| @@ -1883,16 +1883,16 @@ func TestAcceptComparesALinkEntryAsWritten(t *testing.T) { |
| t.Fatalf("the accepted pair: %v", err) |
| } |
| fleet.Close() |
| - // Re-pointed to the same file through a "..": another pair. |
| + // Re-pointed to the same file through a "./": another pair. (A cleaned |
| + // comparison would fold it to the accepted entry and let the build |
| + // through; review fourteen showed a ".." case did not tell them apart.) |
| if err := os.Remove(filepath.Join(state, "current")); err != nil { |
| t.Fatal(err) |
| } |
| - mkdirs(t, filepath.Join(state, "x")) |
| - // Spelt as a string: filepath.Join would clean the ".." away. |
| - if err := os.Symlink("x/../live.sqlite", filepath.Join(state, "current")); err != nil { |
| + if err := os.Symlink("sub/./live.sqlite", filepath.Join(state, "current")); err != nil { |
| t.Fatal(err) |
| } |
| - repointed := linkEntry(filepath.Join(state, "current"), "x/../live.sqlite") |
| + repointed := linkEntry(filepath.Join(state, "current"), "sub/./live.sqlite") |
| if _, err := MeteredFleetStore(o); !errors.Is(err, ErrUnmeterable) || !strings.Contains(err.Error(), repointed) || strings.Contains(err.Error(), entry) { |
| t.Fatalf("the link re-pointed through a ..: %v; want the store refused naming the new pair and not the accepted one", err) |
| } |
| @@ -1912,3 +1912,17 @@ func TestAcceptComparesALinkEntryAsWritten(t *testing.T) { |
| } |
| _ = db |
| } |
| + |
| +// The rendering the watch compares tells one entry from two: a skipped |
| +// name holding a line break does not render as two names, and two names |
| +// do not render as it. (Review fourteen, both reviewers.) |
| +func TestRenderQuotesEveryEntry(t *testing.T) { |
| + a := FleetInventory{Skipped: []string{"/s/a.sqlite\n/s/b.sqlite"}} |
| + b := FleetInventory{Skipped: []string{"/s/a.sqlite", "/s/b.sqlite"}} |
| + if a.render() == b.render() { |
| + t.Fatalf("one name with a line break renders as two:\n%s", a.render()) |
| + } |
| + if !strings.Contains(a.render(), strconv.Quote("/s/a.sqlite\n/s/b.sqlite")) { |
| + t.Fatalf("the entry is not quoted:\n%s", a.render()) |
| + } |
| +} |
diff --git a/internal/ops/litestream_owners.go b/internal/ops/litestream_owners.go| index 8c0cf20e1..fa8b1b28f 100644 |
| --- a/internal/ops/litestream_owners.go |
| +++ b/internal/ops/litestream_owners.go |
| @@ -637,11 +637,13 @@ const strictTreeMaxDepth = 32 |
| // decided before this is wired. |
| // |
| // What it passes over on purpose: an entry that vanishes between being |
| -// listed and being looked at (ordinary churn: SQLite's journals come and |
| -// go); an empty regular file (it holds nothing; the fleet's periodic look |
| -// notices when it stops being empty); a link that leads nowhere (there is |
| -// nothing behind it); and FIFOs, sockets and devices, and links to them |
| -// (not files; a database cannot be one). |
| +// listed and being looked at, or that is a link when listed and something |
| +// else when read (ordinary churn: SQLite's journals come and go, deploys |
| +// swap a link for a directory; the next look sees what is there); an |
| +// empty regular file (it holds nothing; the fleet's periodic look notices |
| +// when it stops being empty); a link that leads nowhere (there is nothing |
| +// behind it); and FIFOs, sockets and devices, and links to them (not |
| +// files; a database cannot be one). |
| func strictTreeDBs(root string) (dbs, skipped, problems []string) { |
| var walk func(dir string, depth int, hidden bool) |
| walk = func(dir string, depth int, hidden bool) { |
| @@ -680,13 +682,16 @@ func strictTreeDBs(root string) (dbs, skipped, problems []string) { |
| problems = append(problems, p+": a link that cannot be read") |
| continue |
| } |
| - target, terr := os.Stat(p) |
| - // The target is read again after the classification: a link |
| - // re-pointed between the two reads would be classified as |
| - // one target and named as another, and is churn instead. |
| - if again, aerr := os.Readlink(p); aerr != nil || again != dest { |
| - continue |
| + // The entry names the target as read, and the target is |
| + // classified by that very spelling (from the link's own |
| + // directory when relative), 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. |
| + spelled := dest |
| + if !filepath.IsAbs(spelled) { |
| + spelled = filepath.Join(dir, spelled) |
| } |
| + target, terr := os.Stat(spelled) |
| // A problem names the path first (nothing compares a |
| // problem with an Accept list); a skipped link is the |
| // injective entry. |
diff --git a/internal/ops/litestream_owners_test.go b/internal/ops/litestream_owners_test.go| index 4acc1e837..d6e3cfc72 100644 |
| --- a/internal/ops/litestream_owners_test.go |
| +++ b/internal/ops/litestream_owners_test.go |
| @@ -687,9 +687,7 @@ func TestStrictTreeNamesALinkToAFile(t *testing.T) { |
| t.Fatalf("re-pointed: skipped %v; want the new target named", skipped) |
| } |
| // A target with a ".." is quoted, not folded: the entry names exactly |
| - // the target as written (a line break would be too; a fixture cannot |
| - // make a link to a name a temp root refuses to hold, so it is not |
| - // built here). |
| + // the target as written. |
| if err := os.Remove(filepath.Join(root, "current")); err != nil { |
| t.Fatal(err) |
| } |
| @@ -703,6 +701,16 @@ func TestStrictTreeNamesALinkToAFile(t *testing.T) { |
| if !contains(skipped, linkEntry(filepath.Join(root, "current"), odd)) || contains(skipped, linkEntry(filepath.Join(root, "current"), other)) { |
| t.Fatalf("a target spelt through ..: skipped %v; want it named as written", skipped) |
| } |
| + // A target holding a line break is quoted too, and the file behind it |
| + // is named as the one file it is. |
| + broken := writeFile(t, filepath.Join(platformDir(t), "elsewhere", "a\nb.sqlite")) |
| + if err := os.Symlink(broken, filepath.Join(root, "newline")); err != nil { |
| + t.Fatal(err) |
| + } |
| + _, skipped, _ = strictTreeDBs(root) |
| + if !contains(skipped, linkEntry(filepath.Join(root, "newline"), broken)) || strings.Contains(strings.Join(skipped, "|"), "\n") { |
| + t.Fatalf("a target with a line break: skipped %q; want it quoted", skipped) |
| + } |
| // A link whose target cannot be looked at is a problem, not nothing. |
| if os.Geteuid() != 0 { |
| locked := filepath.Join(platformDir(t), "locked") |