amadan
The replicator's fourteenth build: the thirteenth review's findings
A skipped link is named `link "path" -> "target"`, one to one, so no file name reads as an accepted link entry (both reviewers); two owned paths whose kept facts are one file are both unowned; a link's target is read again after it is classified; the Accept test names the entry it expects. Comments both listed are corrected. Dispositions "Review thirteen" and the reports.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
7 files changed,
+440
−35
docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review13.md+194 −0docs/superpowers/specs/DISPOSITIONS-metered-replicator.md+17 −2docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review13.md+140 −0internal/ops/litestream_metered.go+5 −4internal/ops/litestream_metered_test.go+27 −9internal/ops/litestream_owners.go+49 −16internal/ops/litestream_owners_test.go+8 −4
CI passed — run details
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review13.md b/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review13.md| new file mode 100644 |
| index 000000000..b51174c09 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-metered-replicator-review13.md |
| @@ -0,0 +1,194 @@ |
| +| Review-twelve finding | Closed at `90032b61`? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Opus P3-1 — ambiguous link acceptance/encoding** | **Partly.** Cleaning no longer collapses targets inside link entries. Target quoting fixes target control characters. Unescaped filenames still permit impersonation and inventory collisions; finding 5 below. | **No** for restoring whole-entry cleaning: `TestAcceptComparesALinkEntryAsWritten` distinguishes it. **Yes** for the remaining filename collisions. | |
| +| **Opus P3-2 — different claimant checks can read different inodes** | **Closed for the reported mismatch.** Later successful checks must match the first check’s device and inode. | **Yes:** no test forces different identities between claimant checks. The fix is established by inspection only. | |
| +| **Opus P3-3 — tree-only mutation survives; root-run vacuity** | **Closed for the missing fixture and misleading root pass.** The second fixture distinguishes tree-only checking; the mixed-root/platform test now explicitly skips under root. Root coverage remains absent. | **No** for tree-only checking. The mixed-UID test fails without its ownership fix when non-root; under root it **skips**, proving nothing. | |
| +| **Opus P3-4 — dropped `Readlink` error** | **Closed by inspection.** ENOENT is churn; other errors become problems. | **Yes:** no test forces this error branch. | |
| +| **Opus claims** | **Mostly corrected.** Remaining contradictions are listed below. | Prose corrections have no behavioral test. | |
| +| **Astra 1 — Litestream pathname opens** | **Open.** | Existing tests do not force substitution at Litestream’s actual opens. | |
| +| **Astra 2 — replaceable ancestry** | **Open.** | Endpoint fixtures still pass without protecting ancestry. | |
| +| **Astra 3 — delayed admission escapes holds** | **Open.** | No test delays an admitted attempt beyond the margin across midnight. | |
| +| **Astra 4 — process exit loses uncertainty** | **Open.** | No durable handover/recovery test exists. | |
| +| **Astra 5 — unsafe acceptance key and inventory encoding** | **Partly**, as Opus P3-1. | The cleaning regression now fails its test; filename impersonation and newline collisions remain untested. | |
| +| **Astra 6 — device-open effects during header inspection** | **Open.** | Final-component fixtures do not exercise ancestor substitution. | |
| +| **Astra 7 — hibernating aliases and skipped-file claims** | **Open.** | No test requires detection of those competing claims. | |
| +| **Astra 8 — required coverage silently omitted** | **Open.** | Missing configured console databases and external-inventory reconciliation have no closing tests. | |
| +| **Astra 9 — ordinary links refuse the whole replacement fleet** | **Open**, explicitly acknowledged. | Tests exercise refusal; none establishes tenant-scoped failure. | |
| +| **Astra 10 — route ownership not independently bound to filesystem ownership** | **Open.** | The coherent-other-owner fixture explicitly **expects acceptance**. | |
| +| **Astra 11 — weak tests** | **Partly.** Tree-only checking and end-to-end acceptance now have distinguishing cases. Shutdown, watch, attribution, interleaving and environment gaps remain. | **Yes** for the surviving mutations in finding 11. | |
| +| **Astra claims** | **Mostly corrected.** Encoding, historical disposition and test-comment overstatements remain. | Author-reported test and mutation runs were not independently executed. | |
| + |
| +All locations below refer to **`90032b61`**. This was a source-only review of both requested diffs. No files were modified, tests run, or network commands issued. |
| + |
| +1. **P1 — Litestream still opens files outside the ownership checks.** |
| + |
| + **Locations:** `internal/lease/meteredstore.go:127`; `internal/ops/litestream_owners.go:360`; `third_party/litestream/db.go:1004`, `:1014`, `:1032`, `:1866`. |
| + |
| + A tenant passes inventory, substitutes another tenant’s database while Litestream initializes, then restores the original pathname before the next look. Litestream can retain the substituted descriptor while every subsequent inventory sees the original inode. Initialization can also change the substituted database through SQLite. Subsequent WAL opens remain independently exposed. |
| + |
| + This is larger than a first-open race or one polling interval. Close’s final pathname comparison cannot detect a substitution already restored. |
| + |
| + **Smallest acceptable fix:** constrain the actual database, SQLite and sidecar opens through pinned directory authority, including subsequent opens. Checking identity after an unrestricted, effects-bearing open is insufficient. |
| + |
| +2. **P1 — Protected endpoints still permit tenant-replaceable ancestry.** |
| + |
| + **Locations:** `internal/ops/litestream_owners.go:490`, `:561`, `:779`, `:807`; `internal/ops/litestream_metered.go:960`. |
| + |
| + T owns `/srv/T`. Beneath it are root-owned `bridge` and tenant-owned `A`. A platform-made link targets `/srv/T/bridge/A`. The target’s immediate parent passes `platformOwned`, but T can rename `bridge` through `/srv/T` and substitute the hierarchy. |
| + |
| + The same construction works above a protected `dataDir`, shadow state, or the directory holding the platform-made link. A platform-made link whose target’s **immediate parent** is tenant-owned is refused; higher replaceable ancestry remains unchecked. |
| + |
| + **Smallest acceptable fix:** establish and pin trusted ancestry for claim roots, permitted platform links and shadow state before accessing anything beneath them. |
| + |
| +3. **P1 — An admitted request can still fall outside every inherited hold.** |
| + |
| + **Locations:** `internal/ops/litestream_metered.go:238`, `:323`, `:762`; `internal/lease/requestgate.go:58`. |
| + |
| + A concrete UTC sequence: |
| + |
| + - 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 is built for the new owner and closed unopened while A remains live. B correctly retains A. |
| + - A’s attempt resumes at **00:02 on October 3**, is recorded, sent and completed. |
| + - 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 corrected lower bound and replacement-chain logic preserve the window they receive. They cannot supply its missing upper bound. |
| + |
| + **Smallest acceptable fix:** track admitted attempts through recording/completion and incorporate their doubtful dates before finalizing handover. Increasing the fixed margin does not close this. |
| + |
| +4. **P1 — Ownership 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 with no predecessor and no durable uncertainty covering the completed attempt. |
| + |
| + A routine restart can also discard inheritance not yet delivered to replacement recorders. An unfinished ledger row covers a different case. |
| + |
| + **Smallest acceptable fix:** persist the relevant ownership/admission uncertainty before affected requests escape, and recover it before establishing successor coverage. |
| + |
| +5. **P2 — Quoting only the target leaves acceptance and inventory encoding ambiguous.** |
| + |
| + **Locations:** `internal/ops/litestream_owners.go:849`, `:855`; `internal/ops/litestream_metered.go:669`, `:860`. |
| + |
| + Two concrete failures remain: |
| + |
| + - Initially, `/srv/u/current` links to relative target `live`. Its accepted entry is `/srv/u/current -> "live"`. Remove that link and create a regular SQLite file whose literal filename is `current -> "live"`. Header inspection places that file in `Skipped` under **exactly the accepted string**. `isLinkEntry` mistakes the filename for a link entry. Both the running inventory and a rebuild can miss the substitution. |
| + - One skipped file can have the pathname `/srv/u/a.sqlite\n/srv/u/b.sqlite`, where `\n` represents an actual newline in the directory name `a.sqlite\n`, followed by nested directories `srv/u`. Its rendered skipped section is identical to two separate skipped files `/srv/u/a.sqlite` and `/srv/u/b.sqlite`. Replacing that hierarchy with those two files need not produce inventory drift. |
| + |
| + Target quoting prevents targets from injecting raw newlines. It does not protect pathnames or distinguish entry kinds. The dispositions acknowledge filename impersonation, but the remaining defect also affects `render()`. |
| + |
| + **Smallest acceptable fix:** use typed entries with separately encoded path and target fields, and an unambiguous serialization for inventory comparison. Normalize only the path field where intended; retain link targets literally. Test file/link impersonation and newline-bearing filenames through both drift detection and rebuild acceptance. |
| + |
| +6. **P2 — Header inspection can still cause device-open effects before rejecting the descriptor.** |
| + |
| + **Location:** `internal/ops/sqliteopen_unix.go:26`. |
| + |
| + The walker observes regular file `T/d/watchdog`. T replaces `d` with a link to `/dev`. On a suitably equipped box, the privileged open reaches `/dev/watchdog`; the later regular-file and identity checks cannot undo its open-side effects. |
| + |
| + `O_NOFOLLOW` protects the final component. `O_NONBLOCK` prevents the demonstrated FIFO wait, and `O_NOCTTY` prevents controlling-terminal acquisition. Neither confines ancestor resolution or supplies a general filesystem deadline. |
| + |
| + **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 — Hibernating aliases and claims on skipped files still hide competing ownership.** |
| + |
| + **Locations:** `internal/ops/litestream.go:381`, `:479`, `:540`, `:709`; `internal/ops/litestream_owners.go:165`. |
| + |
| + A live unit claims `/var/lib/carlos-app/u`, reached through its platform-made state link. Another account’s hibernating exec route names the same database as `/var/lib/private/carlos-app/u/main.db`. The hibernating path contributes no candidate; textual claim matching misses the shared inode. |
| + |
| + A hibernating route naming `u/.cache/app.db` or `u/main.sqlite` can likewise conflict with a unit tree while the file appears only in `Skipped`. Acceptance does not disclose that competing claim. Two hibernating claims also bypass the listed-file conflict checks. |
| + |
| + **Smallest acceptable fix:** reconcile claims by file identity against listed and skipped files, and retain claimant information on skipped entries. |
| + |
| +8. **P2 — Required coverage can still disappear without an inventory entry.** |
| + |
| + **Locations:** `internal/ops/litestream.go:507`, `:525`; `internal/ops/litestream_owners.go:628`, `:638`, `:676`. |
| + |
| + Set `CONSOLE_DB` to a missing file outside the globs and route claims. It is dropped 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, never reaches any list. |
| + |
| + **Smallest acceptable fix:** distinguish required platform databases from optional ones and reconcile against the external replicator’s inventory before cutover. Keep deliberate empty-file/churn exclusions explicit. |
| + |
| +9. **P2 — Ordinary tenant links can still refuse replacement replication for every account.** |
| + |
| + **Locations:** `internal/ops/litestream_owners.go:663`, `:668`; `internal/ops/litestream_metered.go:853`, `:878`. |
| + |
| + A tenant installs a virtual environment or packages containing ordinary links. Links to regular files require individual acceptance; directory links are unconditional problems. The current fleet reports drift, but its replacement refuses. An unrelated ownership change can therefore leave every account waiting for that tenant’s inventory to be resolved. |
| + |
| + Separately, one owned file’s deletion or replacement holds every recorder’s days at `litestream_metered.go:656`. The new cross-check inode refusal has the same scope: a legitimate concurrent rename between checks can conservatively hold everyone. |
| + |
| + **Smallest acceptable fix:** settle and implement the failure scope before wiring—such as isolating affected tenants’ replication and holds—or explicitly adopt the box-wide policy with an operational recovery procedure. |
| + |
| +10. **P3 — A route’s identity still is not bound to independently established filesystem authority.** |
| + |
| + **Locations:** `internal/ops/litestream_owners.go:175`, `:413`, `:472`, `:537`; `internal/ops/litestream_owners_unix_test.go:381`. |
| + |
| + Route A naming B’s coherent directory and file attributes B’s file to A. The fixture explicitly expects this acceptance. |
| + |
| + Root-run units and tenants sharing the process UID can claim unrelated qualifying platform-owned files. For example, a route naming an otherwise unclaimed root-owned SQLite database under protected `/var/lib` directories can pass. Acceptance is **not literally from anywhere**: directory permissions, link rules, filesystem, file type, link count and siblings are still checked. |
| + |
| + A distinct non-platform tenant UID gains no root/process equivalence. A root-owned file directly inside that tenant’s directory is refused, and a tenant-owned file directly inside a protected platform directory is generally refused at 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 does not establish this invariant. |
| + |
| +11. **P3 — Several tests still pass without the behavior they purport to establish.** |
| + |
| + **Locations:** `internal/ops/litestream_metered_test.go:688`, `:828`, `:960`, `:1061`, `:1379`, `:1495`, `:1860`; `internal/ops/litestream_owners_test.go:687`; `internal/ops/litestream_owners_unix_test.go:20`, `:482`. |
| + |
| + Surviving counterfactuals include: |
| + |
| + - Removing `store.Close` still satisfies cleared-pointer and refusing-gate assertions; retained database handles and worker exit are not asserted. |
| + - Removing the watch’s `lookAgain` call leaves the direct-call file-look tests intact. |
| + - Swapping the fleet’s database recorders can satisfy positive counts for both recorders. The lower-level lease test distinguishes its recorders; fleet construction does not have that correspondence assertion. |
| + - 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 the sequential tests. Racing successors are outside the documented caller contract. |
| + - Removing `prepareShadowState`’s data-directory check survives the fixture refused earlier during inventory. |
| + - Removing the new cross-check inode comparison or non-ENOENT `Readlink` reporting has no distinguishing test. |
| + - Replacing `strconv.Quote` with merely adding surrounding quotes preserves the current target fixtures; none contains the newline mentioned in the test comment. |
| + |
| + The old midnight lower-bound fixture at `:1100` still passes without using `lookStart`; newer last-clean fixtures distinguish that defect. The 300 ms negative check can pass incorrectly if the second goroutine has not run. The clean-start test stores Unix nanoseconds, losing monotonic time before its comparisons, so wall-clock adjustment can affect it. |
| + |
| + Under root, the mixed-root/platform test now explicitly skips; unreadable-directory cases and the sealed-target case also do not execute. `.amadan/ci.d/10-gate:51` delegates to `make test`, and `Makefile:18` specifies no UID. **Whether CI runs as root is not established by the permitted committed-source evidence.** |
| + |
| + **Smallest acceptable fix:** add controlled interleavings, retained shutdown references, watch-driven file assertions, per-database recorder correspondence and independent encoded-entry expectations. Exercise permission cases under a known non-root UID or equivalent scoped seams. |
| + |
| +The remaining requested checks hold with these qualifications: |
| + |
| +- **Chain climb, starting directory and platform links:** `litestream_owners.go:450`, `:472`, `:478`, `:487` and `:490` reject an ordinary foreign starting directory, tenant-made links and foreign stopping parents on stable paths. Every accepting `tenantFile` path reaches the top-of-chain checks. A final-component `DBPath` symlink is refused at `:413`. A platform-made link to a target with a tenant-owned immediate parent is refused at `:807`; finding 2 covers higher ancestry. |
| + |
| +- **Platform layouts and all claimant checks:** `claimChecks` at `litestream_owners.go:256` retains multiple tree checks, tree-plus-file checks, and a unit file check alongside an exec `namedFile` check inside `dataDir`. No ranking discards restrictions. Ordinary DynamicUser state links, protected operator-chosen platform paths, and the documented `0700 carlos:carlos` isolated-instance layout can pass. Distinct tenant-owned files in platform directories remain an explicitly undecided refusal. |
| + |
| +- **One-inode rule:** `litestream_owners.go:193` correctly compares later checks with the first successful check and retains the first facts. Thus the stored identity passed every check. Order is immaterial on a stable filesystem; concurrent replacement can cause conservative refusal. This establishes device/inode agreement, not an atomic snapshot of all pathname checks or safe subsequent opens. |
| + |
| +- **Admission and retries:** the stale checks at `litestream_metered.go:212`, `:224` and `:238` cover a file finding before lock acquisition or during the witness read. A finding after the final stale load belongs to the already-admitted gap. `witnessUnchanged` performs at most three reads, immediately respects a witnessed change, tolerates recovered errors and treats exhausted errors as uncertainty. Five-second read contexts do not bound mutex waiting or filesystem `Stat`. |
| + |
| +- **Registry generation:** `registry.go:569` covers all six route columns currently consumed by discovery/attribution. The four triggers at `:602` cover insertion, deletion, changed-value updates and rowid immutability. Older-build raw statements still fire them; the rowid trigger closes the `UPDATE OR REPLACE` deletion escape. Same-value updates, unrelated column updates and Litestream bookkeeping do not move it. Literal “nothing else” is too strong: database-less route insertion/deletion, and some semantically irrelevant changes to those six columns, also move it. |
| + |
| +- **Replacement chain and Close:** `chainClosed` checks each predecessor’s own closure before trusting its latch. Window storage, chain detachment and latch publication occur under `staleMu`; published chain reads use that lock. Within the single-chain contract, a replacement cannot open beside an open or draining predecessor, and an unopened successor cannot latch through one. Inheritance runs once per fleet, including intermediate unopened fleets. Reading a stale predecessor’s window after closure is intentional. |
| + |
| +- **Close’s final look:** the order is store stopped → watch stopped → file look → witness → inheritance → closed (`litestream_metered.go:736`, `:739`, `:762`, `:765`, `:772`, `:773`). A persistent changed or vanished owned file on a current fleet holds from the last clean look. Already-stale fleets skip both checks; a fleet made stale by the look skips the witness. A harmless change after replication stops can conservatively hold everyone, and an unopened fleet can hold back to construction. The inventory walk can block Close without a filesystem deadline. |
| + |
| +- **Lineage:** `ownedReplicaKey` at `litestream_metered.go:937` uses account plus cleaned pathname. Different valid accounts do not share lineage or shadow keys, including after a rename. A→B→A at the same pathname resumes A’s former lineage; changing app within the same account does not create a new key. Separation from the external replicator’s lineage remains unenforced. |
| + |
| +- **Strict inventory:** names are recognized case-insensitively, but uppercase `.DB` is skipped rather than streamed by tree discovery. Every nonempty regular file not already recognized by name receives a header read, including sidecar-shaped names; `sidecarOf` is gone. Unreadable directories, relative/missing unit claims, units without `DBPath`, empty explicit claims and discovered files failing their final look are reported. Explicit exec claims receive the owner checks and do not change external config generation. |
| + |
| +- **Link handling:** classification uses the final `Stat` result, so a link reaching a regular file through a directory link becomes a skipped link entry. Relative targets are rendered exactly as `Readlink` returns them. An unchanged ordinary pair remains accepted across rebuilds; changed target spellings require renewed acceptance. The file/link ambiguity in finding 5 prevents treating the current string-based split as generally correct. |
| + |
| +- **Traversal, fixtures and open-item scope:** the leading-`..` rule permits systemd’s ordinary `../private/...` shape, including multiple leading ascents, and refuses ascents after named components. On stable link-free ancestry, accepted traversal agrees with kernel resolution. Fixture directories whose modes matter are explicitly chmodded under umask `077`; `platformDir` resolves a linked temporary root. Basename-based owner substitution and fixed fake UIDs remain assumptions. The pathname-open gap includes reopens and sidecars; the crash gap includes routine exits; same-filesystem bind mounts remain undetected when only one alias is inventoried. Two listed aliases are rejected by identity. The review-eight, review-nine and review-twelve reports are present. |
| + |
| +Claims the code contradicts: |
| + |
| +- **Safe encoding is only partially fixed:** dispositions `:268` marks the acceptance-key/encoding finding fixed. Its acknowledged filename residue also permits unchanged rendered inventories, not merely confusing display. `isLinkEntry` recognizes a textual pattern, not an entry type. |
| + |
| +- **Another user’s coherent directory is refused:** dispositions `:233` still describes a direct foreign-directory refusal, while `litestream_owners_unix_test.go:381` explicitly expects acceptance. The test introduction is corrected; this disposition is not. |
| + |
| +- **Links are “never followed”:** `litestream_owners_test.go:659` retains that wording. `strictTreeDBs` follows them for metadata through `os.Stat`; it does not walk into or open their contents. |
| + |
| +- **The target test covers a line break:** `litestream_owners_test.go:687` mentions one, but the fixture only introduces `x/..`. |
| + |
| +- **A closed fleet is never unlatched:** `litestream_metered_test.go:1773` overstates the close-order property. A successor closed over a live predecessor deliberately remains closed and unlatched until a later successor can inherit safely. |
| + |
| +- **Claims already corrected:** the removed ranking, the `strictestClaim` reference in `litestream.go`, the “two things” omission count, the unchecked one-inode assertion, the opened-store recorder-correspondence claim and Close’s unconditional final-witness wording are corrected. Author-reported passing runs remain unverified by this 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 4322444eb..cd592bb7c 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md |
| @@ -230,7 +230,7 @@ Reports: `OPUS-REVIEW-metered-replicator-review10.md`, `CODEX-FINDINGS-metered-r |
| | Finding | Disposition | Where | Reproduction | |
| | --- | --- | --- | --- | |
| -| Opus P2-1, Astra 1 (P1): an explicit exec claim outside the activator's directories was a `namedFile`. | Fixed in the eleventh build by ranking the claims' kinds and keeping one; review eleven (Opus P3-1, Astra 5) showed the kinds are not ordered (a tree check and a file check restrict different things), so the twelfth build runs every distinct check the claims imply and refuses on any (`claimChecks`); `namedFile` is kept for `dataDir/*.db` alone, an isolated instance's directory being the instance's own. | `litestream_owners.go` `strictestClaim`, `activatorFile` | `TestExplicitClaimOutsideTheGlobsIsCheckedAsATenantsFile` (owned in the tenant's own chain; refused through the tenant's directory link to another user's directory; refused when named directly in another user's directory); fails with the outside-glob claim checked as a named file (mutation run). | |
| +| Opus P2-1, Astra 1 (P1): an explicit exec claim outside the activator's directories was a `namedFile`. | Fixed in the eleventh build by ranking the claims' kinds and keeping one; review eleven (Opus P3-1, Astra 5) showed the kinds are not ordered (a tree check and a file check restrict different things), so the twelfth build runs every distinct check the claims imply and refuses on any (`claimChecks`); `namedFile` is kept for `dataDir/*.db` alone, an isolated instance's directory being the instance's own. | `litestream_owners.go` `claimChecks` (was `strictestClaim`), `activatorFile` | `TestExplicitClaimOutsideTheGlobsIsCheckedAsATenantsFile` (owned in the tenant's own chain; refused through the tenant's directory link to another user's directory; another user's coherent chain named by a route is accepted, as the code does and Astra 10 keeps open); fails with the outside-glob claim checked as a named file (mutation run). | |
| | Opus P3-1, Astra 6 (P2): a hibernating route over a non-hibernating exec claim, or over a file in a unit tree, was silent. | Fixed: a hibernating path that the platform or any route that is not hibernating also claims (naming it, or a unit tree above it, inside the globs or out) is a problem, when both spell the path the same way [review eleven: a hibernating unit's own file under a hibernating exec claim was dropped, since the unit was not counted live; the twelfth build counts every unit route; an alias spelled through a platform link is still not compared, open]. | `litestream.go` `liveClaim` | `TestExplicitClaimsOutsideTheGlobsAreInventoried` (a unit file, a unit tree's child, an exec claim outside the globs, each under a hibernating route); fails with the report back to unit files alone (mutation run). | |
| | Opus P3-2: a route claiming the platform's own database was silently platform-owned. | Fixed: such a path is unowned ("also claimed by the route X"), which refuses the store; from the twelfth build a unit tree above it counts as a claim too [review eleven, Opus P3-3 and Astra 7]. | `litestream_owners.go` `routeClaiming` | the same test (a route over the registry); fails with the check removed (mutation run). | |
| | Opus P3-3: `Close`'s look ran the whole inventory under the lifecycle lock; an unopened fleet's hold reached back to the build; an inventory error held nothing. | Fixed for the last: a look that cannot be taken at Close holds from the last clean look. The other two are stated in the code: Close takes as long as a look, and a fleet never opened holds from its build. | `litestream_metered.go` `lookNow(final)` | `TestCloseHoldsWhenItsLastLookCannotBeTaken`; fails with the error left to drift (mutation run). | |
| @@ -265,7 +265,7 @@ 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. A regular file whose own name contains ` -> "` and ends in a quote would read as a link entry; that is stated as the residue. 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: 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-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 | |
| @@ -273,3 +273,18 @@ Reports: `OPUS-REVIEW-metered-replicator-review12.md`, `CODEX-FINDINGS-metered-r |
| | 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. | | | |
| Package tests pass under `/tmp` and `~/.cache` (the author's runs, not as root). The full gate on this tip is next; the thirteenth build has not been read by a reviewer. |
| + |
| +## Review thirteen (reviewed `90032b61`; Opus accepted with fixes, Astra rejected) |
| + |
| +Reports: `OPUS-REVIEW-metered-replicator-review13.md`, `CODEX-FINDINGS-metered-replicator-review13.md` (in the tree from the fourteenth build's commit). Both found review twelve's bounded findings closed, with tests that fail without them where a test exists. Both found the link entry still not one to one (a regular file named like the pair read as the accepted link entry; a line break in a plain name could render as two entries); Opus found the one-inode rule stopping short of the listing-time duplicate check, and the link's two reads (Readlink, then Stat) non-atomic. The fourteenth build: |
| + |
| +| 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-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-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. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review13.md b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review13.md| new file mode 100644 |
| index 000000000..03ce220a0 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review13.md |
| @@ -0,0 +1,140 @@ |
| +# Thirteenth review: feat/carloku-metered-replicator-20261001 at `90032b61` |
| + |
| +**Verdict: accept with fixes.** All four Opus findings from review twelve are closed, and so is Astra 5. Of the fixes that have a test, each test fails without its fix. |
| + |
| +This build adds three small weaknesses of its own: |
| +- the link-entry encoding is still not injective (one link and one regular file can render as the same entry); |
| +- the one-inode rule doesn't extend to the cross-file "twice" check; |
| +- one dispositions row and two test comments still say what the code no longer does. |
| + |
| +Outside the link-entry classification, this build changes only comments and `Accept` in the metered fleet. The registry is untouched. So B, C, D, E, I, M and the latch questions stand as review twelve answered them, and Astra 1–4, 8 and 10 still block wiring. |
| + |
| +I read the source with `git show 90032b61:` only and ran nothing. The brief points to the dispositions' "Review seven" section; the section that covers this build is "Review twelve" (`DISPOSITIONS-metered-replicator.md:262`). |
| + |
| +## Review twelve, finding by finding |
| + |
| +| Finding | At `90032b61` | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Opus P3-1 / Astra 5:** link entry not a safe key or encoding | **Closed for targets.** `linkEntry` quotes the target (`litestream_owners.go:849`). `Accept` compares link entries as written (`litestream_metered.go:860-877`). Two link entries can no longer collide: `strconv.Quote` leaves no unescaped `"` inside the quoted target. The link's own name and plain entries are still raw (P3-1). | **No.** `TestAcceptComparesALinkEntryAsWritten` (`litestream_metered_test.go:1860`) fails when link entries are cleaned: `/S/current -> "/S/x/../live.sqlite"` cleans to the accepted string. Its refusals are asserted only by error class (H-1). | |
| +| **Opus P3-2:** checks not bound to one inode | **Closed within one file** (`litestream_owners.go:189-204`). The first check is the file check, because of the sort at `:279`, and its facts are the right ones to keep. It doesn't reach the listing-time "twice" check (P3-2). | No test (stated). | |
| +| **Opus P3-3 / Astra 11:** every-claim test; root vacuity | **Closed.** The C/o/tree.db case (`litestream_owners_unix_test.go:424-450`) exists. The root/platform test skips as root (`:483`). | **No** for the "tree checks only" mutation. "First check only" is still caught by the first case. | |
| +| **Opus P3-4:** `Readlink` error dropped | **Closed** (`litestream_owners.go:652-659`). ENOENT counts as churn; anything else is a problem. A link replaced by a regular file between `Lstat` and `Readlink` returns EINVAL, so it becomes a problem rather than churn. That fails closed and only adds noise. | No test (stated). | |
| +| **Opus claims** | Corrected at every place review twelve listed. Leftovers are listed under Claims below. | n/a | |
| +| **Astra 1–4, 6–10** | **Open**, as stated. Unchanged code. | n/a | |
| +| **Astra 11** (other test gaps) | Open as listed in the dispositions (`:256`). | n/a | |
| +| **Astra claims** | Mostly corrected. The dispositions row at `:233` still claims a refusal its test no longer makes (see Claims). | n/a | |
| + |
| +## Findings |
| + |
| +### P3-1. A regular file can still read as an accepted link entry (V, T) |
| + |
| +**Where:** `litestream_owners.go:855-858` (`isLinkEntry`); plain entries at `:680` and `:691`. |
| + |
| +**Scenario:** |
| +1. A tenant has accepted `/var/lib/carlos-app/u/current -> "live.sqlite"`. |
| +2. The tenant creates a regular SQLite file in `u` literally named `current -> "live.sqlite"`. That is a legal name: it has no `/`. |
| +3. Its plain skipped entry is byte-identical to the accepted link entry. `isLinkEntry` is true, and it is compared as written and accepted. |
| +4. The running fleet does see drift, because the duplicate line changes `render()`. The rebuild passes with no new acceptance. |
| + |
| +**Same root cause:** the link's own name and every plain entry are still unquoted. A name containing `\n` or `, ` makes `render()` and the `unseen` error text (joined by `, ` at `:879`) ambiguous. |
| +- Names can't contain `/`, so a forged line can't carry an absolute path. |
| +- The forgery is cosmetic, not an acceptance bypass. |
| + |
| +**Bound:** a skipped file is neither replicated nor billed. This is stated as the residue. |
| + |
| +**Smallest fix:** make the encoding injective, which is a one-line change. Either: |
| +- quote the path half as well, and start link entries with a prefix no absolute path can have (`link "p" -> "t"`); or |
| +- keep `Skipped` structured (`{path, target, isLink}`) and render it only for display. |
| + |
| +### P3-2. The one-inode rule stops at the file; the "twice" check uses listing-time identities (W) |
| + |
| +**Where:** `litestream_owners.go:121-141` (`os.Stat` and `SameFile`) against `:191-209` (facts from `checkOwnedFile`). |
| + |
| +**Scenario:** |
| +1. P1 and P2 are both in tenant A's tree and are distinct inodes X and Y when they are listed. |
| +2. The check of P1 reads X. |
| +3. A renames P1 over P2. |
| +4. The check of P2 reads X. |
| +5. Both are owned, with facts X. P1 no longer exists, and litestream's open of P1 by path creates a fresh root-owned database in A's directory. |
| + |
| +**Bound:** |
| +- Only one account is affected: a rename can't cross into a directory the tenant can't write. |
| +- The next look sees P1 gone and goes stale. |
| + |
| +**Smallest fix:** after the loop, refuse any two owned paths whose facts share `Dev`/`Ino`. That is the same rule as `:134`, applied to the facts that are kept. |
| + |
| +### P3-3. Link classification reads the target twice, non-atomically (K, T) |
| + |
| +**Where:** `litestream_owners.go:652` (`Readlink`) and `:661` (`Stat`). |
| + |
| +**Scenario:** |
| +- `p -> "A"` was accepted while A was a regular file. A is now a directory, so `p` should be a problem. |
| +- At build time the tenant re-points `p` to a file F between `Readlink` and `Stat`, then back. |
| +- The entry names A, the classification says "regular", and the accepted pair passes. |
| + |
| +This needs a tenant-timed race during the build, and whatever sits behind A is the tenant's own data. The EINVAL noise noted under Opus P3-4 comes from the same two-step read. |
| + |
| +**Smallest fix:** call `Readlink` again after `Stat` and treat a mismatch as churn or a problem. Treat EINVAL as churn after a fresh `Lstat`. |
| + |
| +### P3-4. The new `Accept` test asserts refusal by class only (H) |
| + |
| +**Where:** `litestream_metered_test.go:1871-1872` and `:1893-1894`. |
| + |
| +Both refusals check only `errors.Is(err, ErrUnmeterable)`. Any other unmeterable condition at the third build would satisfy the test without the as-written comparison. Nothing in the fixture produces one today, so this is a robustness gap, not a vacuous test. |
| + |
| +**Smallest fix:** assert that the error text contains `linkEntry(current, odd)` and does not contain the accepted entry. |
| + |
| +## The lettered questions (code unchanged since `c8e5e47b` unless noted) |
| + |
| +- **A, J, U (the chain, `:402-499`, `:537`):** unchanged. |
| + - An own-uid tenant cannot get a chain holding its link or another user's directory accepted (`:450-462`, `:473`, `:478`, `:487`). The check at the top of the chain is reached on every accepting path. |
| + - A platform-uid tenant, or a unit run as root, stops at once. Its file is accepted wherever the file's directory and the directory above it are platform-owned and not group- or world-writable. That is open with approval H, and it combines with Astra 10: a route can name any root-owned file in a root directory. |
| + - A root-owned file in a tenant's directory is refused at `:461`. A tenant's file in a platform directory is refused at `:537`, stated as not yet decided. |
| + - A `DBPath` that is itself a link is refused at `:414`. |
| + - A platform-made link resolves only through `resolvePlatformLinks`. Higher replaceable ancestry is Astra 2 (open). |
| +- **B, I (`admit`, `:211-245`):** the stale checks at `:212`, `:224` and `:238` are complete. The retries are bounded at `witnessAttempts`. Only a witnessed change, or an error that persists, counts as a change. |
| + - The sleeps are taken under `f.mu`. |
| + - A look that goes stale after `:238` is the admitted gap (Astra 3, open). |
| + - `closeOnce` holds, and `closed` is set last (`:773`). |
| +- **C:** registry triggers unchanged. The answer stands, including database-less route rows moving the counter. |
| +- **D, M, latch:** Close's order is unchanged: store, watch, look, witness, `inheritHold`, closed (`:718-773`). The inherited window is applied once. A day can still go unheld in two ways: |
| + - Astra 3's sequence: admitted at 23:58, the change, a successor closed unopened, recorded at 00:02. 3 October is held by nobody. |
| + - A swap made and restored between the last clean look and Close's look. |
| + - Both are stated as open. |
| +- **E:** the key is account plus path (`litestream_metered.go:937`). A→B→A at one path resumes A's lineage, which is stated. No path is shared across accounts. |
| +- **F, L, N:** nothing is newly silent. |
| + - Unit routes with no `DBPath` or a relative one are reported (`litestream.go:367-378`). |
| + - Vanished or empty claimed files are reported (`:521-536`). |
| + - Explicit claims go through `claimChecks` and never widen what is accepted. |
| + - The external config is unchanged. |
| +- **G:** stated correctly, with Astra's widenings (reopens and sidecars, routine exit) already in the dispositions. |
| +- **K:** `O_NOFOLLOW` only covers the final component, so the device open through a swapped ancestor is Astra 8 (open). `sidecarOf` no longer exists. Platforms without `O_NOFOLLOW` refuse the header read. |
| +- **P, S:** see Opus P3-3 above for the closure. |
| + - Order matters only through the one-inode rule (first facts kept), and the file-first order is the right one for it. |
| + - The DynamicUser and `instances/<id>` layouts pass. |
| +- **Q:** hibernating aliases and hibernating claims on skipped files are still silent (open). |
| +- **R:** link noise is still the box-wide decision to make before wiring. Directory links remain unacceptable problems. A link through a directory link is classified by its final target, which is correct. |
| +- **T:** an unchanged pair stays accepted across rebuilds while the root's spelling is stable. Relative targets render as quoted `Readlink` text. |
| + - The pair pins the spelling, not the file reached. An absolute target through a tenant link outside the tree, or a replaced target file, reads the same. |
| + - That matches plain skipped files, whose contents are never pinned either. |
| +- **W:** first-check facts are right. |
| + - A refusal at a running look turns into `goStale` and holds every recorder. |
| + - Any inode change between looks already did that, so the rule adds no new class of hold, only the same hold seen earlier. |
| + - At build, the refusal is transient. |
| +- **X:** these tests are skipped or vacuous as root: |
| + - `TestRootAndThePlatformUserAreOneOwner`; |
| + - the sealed-link case (`litestream_owners_test.go:703`); |
| + - the unreadable-directory cases (`:425`, `litestream_metered_test.go:664`). |
| + |
| + Nothing in the tree says which user CI runs as. `.amadan/README.md` says only that jobs run in a bubblewrap sandbox, which suggests non-root but doesn't prove it. The "not as root" in the dispositions describes the author's runs. |
| +- **O:** the review-eight through review-twelve reports are in the tree. |
| + |
| +## Claims the code contradicts |
| + |
| +- **`DISPOSITIONS-metered-replicator.md:233`:** the Where column still names `strictestClaim`, a function that no longer exists. The Reproduction column still says "refused when named directly in another user's directory". The test now asserts acceptance (`litestream_owners_unix_test.go:380-387`). Row `:273` says "this file's rows above" were corrected. |
| +- **`litestream_owners_test.go:229` and `:659`:** "a link is never followed". The walk `Stat`s through it to classify it (`litestream_owners.go:661`), as the source comment at `:647` says correctly. |
| +- **`litestream_owners.go:650-651`:** "no target can forge another entry or a line break". True of the target. The entry as a whole can still be forged through the link's own name, or by a regular file's name (P3-1). |
| +- **`MeteredFleetStore` comment `:856-859`, "the pair the operator accepted is the pair that is there":** it is the spelled pair, not the file reached (T). |
| +- **Dispositions, "Package tests pass … not as root":** these are the author's runs, neither confirmed nor contradicted here. |
| + |
| +**Verdict: accept with fixes.** P3-1 to P3-4 and the stale claims should be fixed. 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 9582e63d5..b6927ba14 100644 |
| --- a/internal/ops/litestream_metered.go |
| +++ b/internal/ops/litestream_metered.go |
| @@ -853,10 +853,11 @@ func MeteredFleetStore(o MeteredFleetOptions) (fleet *MeteredFleet, err error) { |
| if len(inv.Problems) > 0 { |
| return nil, fmt.Errorf("%w: places a database could be that cannot be vouched for: %s", ErrUnmeterable, strings.Join(inv.Problems, "; ")) |
| } |
| - // A plain path is accepted as cleaned; a link entry (path -> "target") |
| - // as written, so the pair the operator accepted is the pair that is |
| - // there, and a target that folds or pops under cleaning cannot pass |
| - // as it. |
| + // A plain path is accepted as cleaned; a link entry (link "path" -> |
| + // "target") as written, so the spelled pair the operator accepted is |
| + // the spelled pair that is there (what the target reaches is not |
| + // pinned, as a plain skipped file's contents are not), and a target |
| + // that folds or pops under cleaning cannot pass as it. |
| accepted := map[string]bool{} |
| for _, p := range o.Accept { |
| if isLinkEntry(p) { |
diff --git a/internal/ops/litestream_metered_test.go b/internal/ops/litestream_metered_test.go| index 0484a9a7b..3fe138fac 100644 |
| --- a/internal/ops/litestream_metered_test.go |
| +++ b/internal/ops/litestream_metered_test.go |
| @@ -10,6 +10,7 @@ import ( |
| "net/http/httptest" |
| "os" |
| "path/filepath" |
| + "strconv" |
| "strings" |
| "sync" |
| "sync/atomic" |
| @@ -1769,8 +1770,9 @@ func (c *closedWhenHeld) Incomplete(at time.Time) { |
| // Closing a fleet that was never opened still latches its chain, inside |
| // Close, before the fleet reads as closed: the fleet's own recorders are |
| // told its predecessor's days while the fleet is still open to a |
| -// replacement's eyes, so the window is never read from a fleet that is |
| -// closed and not yet latched. (Review nine, Opus P3-4, and review ten: |
| +// replacement's eyes (a successor closed over a predecessor still live |
| +// stays closed and unlatched on purpose, and a later successor reads |
| +// through it). (Review nine, Opus P3-4, and review ten: |
| // the chain tests drove inheritHold by hand and pinned nothing in |
| // closeNow, nor its order.) |
| func TestCloseOfAnUnopenedFleetLatchesItsChain(t *testing.T) { |
| @@ -1865,15 +1867,16 @@ func TestAcceptComparesALinkEntryAsWritten(t *testing.T) { |
| mkdirs(t, state) |
| db := writeFile(t, filepath.Join(state, "main.db")) |
| live := writeFile(t, filepath.Join(state, "live.sqlite")) |
| - if err := os.Symlink(live, filepath.Join(state, "current")); err != nil { |
| + // A relative target, as tenant software makes them. |
| + if err := os.Symlink("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) |
| - if _, err := MeteredFleetStore(o); !errors.Is(err, ErrUnmeterable) { |
| - t.Fatalf("a link to a file not accepted: %v; want the store refused", err) |
| + entry := linkEntry(filepath.Join(state, "current"), "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) |
| } |
| - entry := linkEntry(filepath.Join(state, "current"), live) |
| o.Accept = []string{entry, live} // live.sqlite is itself a skipped name |
| fleet, err := MeteredFleetStore(o) |
| if err != nil { |
| @@ -1886,11 +1889,26 @@ func TestAcceptComparesALinkEntryAsWritten(t *testing.T) { |
| } |
| mkdirs(t, filepath.Join(state, "x")) |
| // Spelt as a string: filepath.Join would clean the ".." away. |
| - if err := os.Symlink(state+"/x/../live.sqlite", filepath.Join(state, "current")); err != nil { |
| + if err := os.Symlink("x/../live.sqlite", filepath.Join(state, "current")); err != nil { |
| t.Fatal(err) |
| } |
| - if _, err := MeteredFleetStore(o); !errors.Is(err, ErrUnmeterable) { |
| - t.Fatalf("the link re-pointed through a ..: %v; want the store refused until the new pair is accepted", err) |
| + repointed := linkEntry(filepath.Join(state, "current"), "x/../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) |
| + } |
| + // A regular database file whose own name spells the accepted pair as |
| + // an unprefixed encoding would (a legal file name: no separator in it) |
| + // reads as a plain skipped file, never as the accepted link entry |
| + // (review thirteen, both reviewers). |
| + if err := os.Remove(filepath.Join(state, "current")); err != nil { |
| + t.Fatal(err) |
| + } |
| + impostor := filepath.Join(state, "current -> "+strconv.Quote("live.sqlite")) |
| + if err := os.WriteFile(impostor, append([]byte("SQLite format 3\x00"), make([]byte, 100)...), 0o644); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := MeteredFleetStore(o); !errors.Is(err, ErrUnmeterable) || !strings.Contains(err.Error(), impostor) || strings.Contains(err.Error(), "link \"") { |
| + t.Fatalf("a database named like the accepted link's entry: %v; want it refused as the plain file it is", err) |
| } |
| _ = db |
| } |
diff --git a/internal/ops/litestream_owners.go b/internal/ops/litestream_owners.go| index 00c2e7ba4..8c0cf20e1 100644 |
| --- a/internal/ops/litestream_owners.go |
| +++ b/internal/ops/litestream_owners.go |
| @@ -10,6 +10,7 @@ import ( |
| "sort" |
| "strconv" |
| "strings" |
| + "syscall" |
| "github.com/carlosframework/platform/internal/registry" |
| "github.com/carlosframework/platform/internal/release" |
| @@ -208,6 +209,27 @@ func fleetInventory(routes []registry.Route, registryPath, dataDir string) (Flee |
| inv.Owned = append(inv.Owned, OwnedDB{Path: p, Account: owner.Account, App: owner.App, Host: owner.Host, Platform: owner.Account == release.OpsAccount}) |
| inv.facts[p] = facts |
| } |
| + // Two owned paths whose kept facts are one file (a rename between the |
| + // listing and the checks): the listing-time identity check could not |
| + // see it, and litestream opening the gone path by name would make a |
| + // fresh database there. Both are unowned, as two listed names of one |
| + // file are. |
| + byInode := map[[2]uint64][]string{} |
| + for _, o := range inv.Owned { |
| + f := inv.facts[o.Path] |
| + byInode[[2]uint64{f.Dev, f.Ino}] = append(byInode[[2]uint64{f.Dev, f.Ino}], o.Path) |
| + } |
| + kept := inv.Owned[:0] |
| + for _, o := range inv.Owned { |
| + f := inv.facts[o.Path] |
| + if len(byInode[[2]uint64{f.Dev, f.Ino}]) > 1 { |
| + unowned(o.Path, "another owned path became this same file under the look") |
| + delete(inv.facts, o.Path) |
| + continue |
| + } |
| + kept = append(kept, o) |
| + } |
| + inv.Owned = kept |
| sort.Slice(inv.Owned, func(i, j int) bool { return inv.Owned[i].Path < inv.Owned[j].Path }) |
| sort.Strings(inv.Unowned) |
| return inv, nil |
| @@ -646,26 +668,35 @@ func strictTreeDBs(root string) (dbs, skipped, problems []string) { |
| case fi.Mode()&os.ModeSymlink != 0: |
| // Classified by where it leads (Stat follows it for that |
| // and nothing else: it is never walked into or opened). |
| - // A link entry is named with its target, quoted, so a |
| - // link re-pointed after it was accepted reads as a change |
| - // and no target can forge another entry or a line break. |
| + // A link entry is named with its path and target, each |
| + // quoted behind a word no path begins with, so a link |
| + // re-pointed after it was accepted reads as a change and |
| + // no name, of a link or of a file, reads as another entry. |
| dest, rerr := os.Readlink(p) |
| - if errors.Is(rerr, fs.ErrNotExist) { |
| - continue // gone under the look: churn |
| + if errors.Is(rerr, fs.ErrNotExist) || errors.Is(rerr, syscall.EINVAL) { |
| + continue // gone, or no longer a link, under the look: churn |
| } |
| if rerr != nil { |
| problems = append(problems, p+": a link that cannot be read") |
| continue |
| } |
| - entry := linkEntry(p, dest) |
| 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 |
| + } |
| + // A problem names the path first (nothing compares a |
| + // problem with an Accept list); a skipped link is the |
| + // injective entry. |
| switch { |
| case sqliteName(name) || (terr == nil && target.IsDir()): |
| - problems = append(problems, entry+": a link that may lead to a database") |
| + problems = append(problems, p+" -> "+strconv.Quote(dest)+": a link that may lead to a database") |
| case terr != nil && !errors.Is(terr, fs.ErrNotExist): |
| - problems = append(problems, entry+": a link whose target cannot be looked at") |
| + problems = append(problems, p+" -> "+strconv.Quote(dest)+": a link whose target cannot be looked at") |
| case terr == nil && target.Mode().IsRegular(): |
| - skipped = append(skipped, entry) |
| + skipped = append(skipped, linkEntry(p, dest)) |
| } |
| case fi.IsDir(): |
| if depth+1 > strictTreeMaxDepth { |
| @@ -842,17 +873,19 @@ func platformUID(uid uint32) bool { |
| return uid == 0 || int(uid) == os.Geteuid() |
| } |
| -// linkEntry is how a link is named in an inventory: its path, then its |
| -// target as Readlink gave it, quoted. The quoting keeps a target with a |
| -// separator, a line break or a ".." from reading as another path, and |
| -// Accept compares such an entry as written (isLinkEntry), never cleaned. |
| +// linkEntry is how a link is named in an inventory: the word link, then |
| +// its path and its target as Readlink gave it, each quoted. No path can |
| +// begin with that word and a space (a path is absolute), and the quoting |
| +// keeps a separator, a line break or a ".." in either half from reading |
| +// as another entry, so the encoding is one to one: no regular file's name |
| +// and no other link's pair renders the same. Accept compares such an |
| +// entry as written (isLinkEntry), never cleaned. |
| func linkEntry(path, target string) string { |
| - return path + " -> " + strconv.Quote(target) |
| + return "link " + strconv.Quote(path) + " -> " + strconv.Quote(target) |
| } |
| // isLinkEntry reports whether an inventory entry names a link with its |
| // target (linkEntry). |
| func isLinkEntry(entry string) bool { |
| - i := strings.Index(entry, " -> \"") |
| - return i > 0 && strings.HasSuffix(entry, "\"") |
| + return strings.HasPrefix(entry, "link \"") |
| } |
diff --git a/internal/ops/litestream_owners_test.go b/internal/ops/litestream_owners_test.go| index ae0ddf05f..4acc1e837 100644 |
| --- a/internal/ops/litestream_owners_test.go |
| +++ b/internal/ops/litestream_owners_test.go |
| @@ -226,7 +226,8 @@ func TestFleetDBOwnersFindTheRegistryWhereItIsOpen(t *testing.T) { |
| } |
| // A unit tenant owns its state directory and can put a link in it. A link |
| -// is never followed: one named like a database, or one that leads to a |
| +// is never walked into or opened (one Stat classifies it by where it |
| +// leads): one named like a database, or one that leads to a |
| // directory, is reported as a place a database could be that cannot be |
| // vouched for, and the replicator refuses to start. Another tenant's |
| // database reached through such a link is not listed under this tenant at |
| @@ -656,7 +657,8 @@ func TestExplicitClaimsOutsideTheGlobsAreInventoried(t *testing.T) { |
| // A link in a unit tree that leads to a regular file under a name that is |
| // not a database's (current -> live.sqlite) is named to the caller as |
| -// skipped, never followed and never passed over. (Review ten, Astra 8.) |
| +// skipped, never walked into or opened and never passed over. (Review |
| +// ten, Astra 8.) |
| func TestStrictTreeNamesALinkToAFile(t *testing.T) { |
| root := platformDir(t) |
| target := writeFile(t, filepath.Join(platformDir(t), "elsewhere", "live.sqlite")) |
| @@ -684,8 +686,10 @@ func TestStrictTreeNamesALinkToAFile(t *testing.T) { |
| if !contains(skipped, linkEntry(filepath.Join(root, "current"), other)) || contains(skipped, linkEntry(filepath.Join(root, "current"), target)) { |
| t.Fatalf("re-pointed: skipped %v; want the new target named", skipped) |
| } |
| - // A target with a line break or a ".." is quoted, not folded: the |
| - // entry names exactly the target as written. |
| + // 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). |
| if err := os.Remove(filepath.Join(root, "current")); err != nil { |
| t.Fatal(err) |
| } |