Skip to content

review: test fix, style, cuts, docs - #313

Merged
CMGS merged 4 commits into
mainfrom
review/whole-repo-1005
Oct 4, 2026
Merged

CMGS merged 4 commits into
mainfrom
review/whole-repo-1005

Conversation

@CMGS

@CMGS CMGS commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Review round on main b778c66, covering the 21 files that #310, #311 and #312 changed. Every other file is unchanged since the last round and keeps its ledger verdict.

What changed

  • Test fix (07d4504). NOTIFY channels are database-wide, so another package writing the same cell name could fire TestWatchFiresOnConnectAndOnItsCellsWrites inside its quiet window. The test now listens on a cell named after its own schema.
  • Style (9ce057b).
    • errors.Is(err, fs.ErrNotExist) replaces os.IsNotExist in store/dir.
    • %w wraps the key error in Manager.validate.
    • A test body uses t.Context().
    • The store/dir package doc is one line.
  • Cuts (cabb7a6), behavior-preserving:
    • The egress fingerprint sorts the secrets as loaded. Validation already rejects value, so the field-by-field copy produced the same digest bytes.
    • metastore.Listen holds its one-connection loop inline, so the callback types are written once. It passes the config straight to ConnectConfig, which copies it.
    • handleListCheckpoints and handlePutPools answer through writeResult.
    • hashExport drops the chunk count, since one errgroup task per chunk already bounds the workers.
    • sweepGenerations has one ReadDir loop with a keep set.
    • tenants/pg: an entry is present when it holds a record (the present flag was always rec != nil).
    • The Metas benchmark seeds through mustPublish.
  • Docs (057acda). A node with meta_store needs the database at boot, because both shared sets open it there. pools.json is served when the row read fails after the connection succeeds, and kept if the node leaves the cell.

Net: prod +47/−83 (−36), test +13/−17 (−4). Comment lines +2/−4. No hot path is touched: the cuts are in config load, the pool-set listener, two handlers, and the store's cold sweep and export paths.

Owner calls (not applied)

  1. Boot without the database. With meta_store, PostgreSQL must be reachable at boot (tenants/pg and poolset/pg both run ensureTable in Open). A pools.json fallback for "database down at boot" would need both stores to open lazily. The docs now state the real behavior.
  2. adoptPools reusing desiredPools. About −5 prod lines and one validation path. A hand-edited pools.json carrying config-owned fields or a duplicate key would then refuse to boot instead of being tolerated, and the error text changes.
  3. [contrived] Pool-set version regression after an async failover that loses a committed write. Nodes that applied the lost version skip the next write that reuses its number. This needs asynchronous replication that drops an acknowledged commit. Not changed.

Evidence

  • GOWORK=off make go-lint: 10 × 0 issues.
  • asl -forwarder=false ./...: clean on darwin and linux for sandboxd, sdk/go, e2e, protocol/wire and mcp. The advisory forwarder findings (skipIdle, refillGated, benignSweepErr) are already KEPT in the ledger.
  • go test -race -count=1 ./... (sandboxd) passes:
    • on macOS, with SANDBOXD_TEST_PG_URL set;
    • in a golang:1.27.1 linux/arm64 container with PostgreSQL (as non-root, plus . and mesh as root because of a read-only module cache).

CMGS added 4 commits October 5, 2026 00:48
NOTIFY channels are database-wide, so a parallel package writing the same
cell name could fire the watch inside its quiet window.
….Context in a test body, one-line dir package doc
- config: the egress fingerprint sorts the secrets as loaded; validation
  already rejects a value, so the field-by-field copy changed nothing
- metastore: Listen holds its one-connection loop inline and passes the
  config straight to ConnectConfig, which copies it itself
- server: list checkpoints and put pools answer through writeResult
- store/dir: hashExport drops the chunk count (one task per chunk already
  bounds the workers); sweepGenerations has one ReadDir loop with a keep set
- tenants/pg: an entry is present when it holds a record
- store/dir test: the Metas benchmark seeds through mustPublish
@CMGS CMGS changed the title review: whole-repo round on b778c66 (test fix, style, cuts −36 prod, docs) review: test fix, style, cuts, docs Oct 4, 2026
@CMGS
CMGS merged commit 3c880f5 into main Oct 4, 2026
1 check passed
@CMGS
CMGS deleted the review/whole-repo-1005 branch October 4, 2026 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant