amadan
The reader fix's test closes its registry and claims only what it checks; both reviews recorded
Reviewed at 51587e00: Opus accepted, Astra accepted with fixes. Both found the defect real on all five failing paths and the new outcome the conservative one (an unreadable registry leaves every sampled owner unknown, where the panic stopped monitoring for the process). The two P3s were about the test: it never closed its registry, and its comment said the connection is released, which it does not check.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
3 files changed,
+72
−3
docs/superpowers/specs/CODEX-FINDINGS-observe-routes-cleanup-review1.md+15 −0docs/superpowers/specs/OPUS-REVIEW-observe-routes-cleanup-review1.md+52 −0internal/registry/observe_test.go+5 −3
CI passed — run details
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-observe-routes-cleanup-review1.md b/docs/superpowers/specs/CODEX-FINDINGS-observe-routes-cleanup-review1.md| new file mode 100644 |
| index 000000000..17c996a32 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-observe-routes-cleanup-review1.md |
| @@ -0,0 +1,15 @@ |
| +- **P3 — `internal/registry/observe_test.go:224`: the new test leaks its setup registry.** `Open(path)` creates a long-lived database pool, but the test never closes `reg`. `ObserveRoutes` closes only its separate observation connection and pool. Repeated test runs therefore accumulate open handles. Smallest fix: add `defer reg.Close()` immediately after the successful `Open` check. |
| + |
| +The requested checks otherwise support the fix: |
| + |
| +1. **All five failure paths were defective at `12e70d95`.** PRAGMA, BEGIN, `data_version`, row-read and COMMIT failures all execute `return nil, err`, clearing `obs` before the old deferred `obs.Close()` dereferences it. At `51587e00`, `observe.go:48–55` retains `built`; each path returns the original error and nil observation, invoking connection close and database close exactly once through `Close` (`observe.go:100–112`). Database close still executes if connection close returns an error. Cleanup errors are discarded; they do not replace the triggering error. |
| + |
| +2. **Success behavior is unchanged.** The same observation remains open for the caller, with identical rows, witness and transaction behavior. No caller or test relies on the panic. Specifically, `registryOwnerSnapshot` (`internal/host/agent.go:2510`) uses `List` and is unaffected; the live metrics caller is `registryOwnerObservation` at line 2524. It logs the returned error and returns `ok=false`. `unitScanOwnedOnce` (`internal/host/metrics_owner.go:374–402`) then marks every sampled owner unknown and withholds coverage and ownership proof; CPU ownership proof is also false. This is conservative: unreadable ownership never becomes attribution. Previously, the metrics loop recovered the panic and stopped monitoring for that process (`internal/host/metrics_loop.go:863`). There is no `ObserveRoutes` use in `internal/edge`. |
| + |
| +3. **The new test would fail without the fix.** Dropping `routes` reaches the row-read failure after opening the connection and transaction, causing the old deferred cleanup to panic. It uses a private temporary SQLite database, with no external service, credentials, subprocess or timing dependency. It verifies error/nil behavior, but does **not** independently assert resource closure. |
| + |
| +4. **No remaining instance of the named-result cleanup bug was found** in `observe.go` or elsewhere under `internal/registry`, including tests. The remaining deferred closes capture their receivers when registered; the corrected closure captures `built`. |
| + |
| +Source review only; no modifications, tests or network commands. |
| + |
| +**Verdict: accept with fixes.** |
| \ No newline at end of file |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-observe-routes-cleanup-review1.md b/docs/superpowers/specs/OPUS-REVIEW-observe-routes-cleanup-review1.md| new file mode 100644 |
| index 000000000..e3d4c6596 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-observe-routes-cleanup-review1.md |
| @@ -0,0 +1,52 @@ |
| +**Verdict: accept.** I found no P1 or P2 issues, only two P3s in the test. |
| + |
| +## Findings |
| + |
| +**P3: the test comment claims the connection is released, but the test never checks it** (`internal/registry/observe_test.go:218-235`) |
| +- **Scenario:** a later change makes the cleanup close only `conn` and not `db`, or drops the close entirely but still returns `(nil, err)`. The test still passes, because it only asserts `err != nil && obs == nil`. It does pin the panic, which is the defect being fixed, but not the "released" half of its own comment. |
| +- **Smallest fix:** trim the comment to "is an error, not a panic". Or, if the leak matters, add a check that can see it. |
| + |
| +**P3: the test never closes `reg`** (`observe_test.go:224`) |
| +- **Scenario:** a harmless handle leak until the process exits. `t.TempDir` cleanup still succeeds on Linux. |
| +- **Smallest fix:** `defer reg.Close()`. Optional. |
| + |
| +## The four questions |
| + |
| +**1. The defect was real at 12e70d95 on every path, and is fixed on every path.** |
| +- At 12e70d95, every return after the defer is armed is `return nil, err`: |
| + - PRAGMA busy_timeout (line 59) |
| + - BEGIN (line 67) |
| + - the data_version read (line 70) |
| + - the row read (line 75) |
| + - COMMIT (line 80) |
| +- On each of them Go assigns `obs = nil` before the deferred function runs. The old cleanup then called `(*RouteObservation)(nil).Close()`, and `Close` reads `o.conn` on its first line (`observe.go:102`): a nil dereference. The connection and the `*sql.DB` both leaked. |
| +- At 51587e00 the cleanup closes `built`, which is never reassigned. `Close` closes `conn` and then `db`, and nils both fields, so each closes exactly once. The cleanup is the only `Close` on these paths. |
| +- The two earlier returns (lines 36 and 42) come before the defer and close what they opened by hand; they are unchanged and correct. |
| +- On the data_version and row-read failures, ROLLBACK runs before the close. On a COMMIT failure there is no ROLLBACK, but closing the connection ends the read transaction. |
| + |
| +**2. Nothing changes on the success path. The new error is the conservative outcome, and is better than the old one.** |
| +- **Success path:** `obs` and `built` are the same pointer, `err` is nil, the cleanup does nothing, and `return obs, nil` hands back the same object as before. Lines 70 and 75 still write `version` and `Routes` through `obs`, which is fine because it still points at `built` there. |
| +- **Callers:** the only production caller is `registryOwnerObservation` (`internal/host/agent.go:2524`). It is wired once, into `metricsLoop`, at `agent.go:513`. |
| + - `registryOwnerSnapshot` does not call `ObserveRoutes`; it uses `reg.List()`. The brief names the wrong function. |
| + - `internal/edge` does not use `ObserveRoutes`. |
| + - No test relied on the panic. `TestObserveRoutesBrokenFile` fails at `db.Conn`, before the defer is armed, so it never reached the bug. |
| +- **Before the fix:** the panic was caught by `metricsLoop`'s `recover` (`metrics_loop.go:863-867`), which logs "monitoring stopped for this process" and returns. So nothing was misattributed, but metrics and alerts were dead until the agent restarted. The connection, the `*sql.DB` and the scanner's held `prev` observation all leaked. |
| +- **Now:** the error is logged and `ok=false` is returned. |
| + - `unitScanOwnedOnce` sets `okBefore=false`. Coverage is false, ownership is false, and every unit gets an unknown owner (`metrics_owner.go:382, 396-400`). |
| + - `run` sets `s.prev = last = nil`, so the next tick's CPU interval is also unproven. |
| + - The loop keeps sampling, and no sample is attributed while ownership could not be read. That is the conservative outcome sample-time ownership requires. |
| + |
| +**3. The test fails without the fix and depends only on the code under test.** |
| +- No foreign key, view or trigger references `routes`, so `DROP TABLE routes` succeeds. |
| +- `ObserveRoutes` opens its own `sql.Open`, not `Open`, so nothing recreates the table. |
| +- The PRAGMA, BEGIN and data_version all succeed. The SELECT fails with "no such table", which takes the line-75 return. |
| +- At 12e70d95 that path panics in `Close`, and the test binary fails. At 51587e00 it returns `(nil, err)`, and the test passes. |
| +- Its only other dependencies are `Open`'s schema and the SQLite driver, which every test in this file already uses. |
| +- It exercises one of the five paths. The other four share the same cleanup, so one test covers the mechanism. |
| + |
| +**4. There are no other instances of this shape in observe.go or anywhere under internal/registry.** |
| +- Outside tests, `observe.go:50` is the only `defer func()` in `internal/registry`. |
| +- The only other function there with named results is `CreateFleet` (`fleets.go:178`). It has no defer. |
| +- `FleetPlacement` (`fleets.go:438`) also has named results but no defer. |
| + |
| +**Verdict: accept.** The two P3s are optional test tidy-ups. |
diff --git a/internal/registry/observe_test.go b/internal/registry/observe_test.go| index 83890ab31..b2513248c 100644 |
| --- a/internal/registry/observe_test.go |
| +++ b/internal/registry/observe_test.go |
| @@ -216,15 +216,17 @@ func TestObserveRoutesHelperProcess(t *testing.T) { |
| } |
| } |
| -// A read that fails after the connection has opened is an error, and the |
| -// connection is released. It used to be a panic: the cleanup closed the |
| -// named result, which the failing return had already cleared. |
| +// A read that fails after the connection has opened is an error. It used |
| +// to be a panic: the cleanup closed the named result, which the failing |
| +// return had already cleared. (That the connection is released on this |
| +// path is by reading the code; this test does not see it.) |
| func TestObserveRoutesFailingReadIsAnErrorNotAPanic(t *testing.T) { |
| path := filepath.Join(t.TempDir(), "registry.db") |
| reg, err := Open(path) |
| if err != nil { |
| t.Fatal(err) |
| } |
| + defer reg.Close() |
| // The file opens and answers PRAGMAs; the rows cannot be read. |
| if _, err := reg.sql.Exec(`DROP TABLE routes`); err != nil { |
| t.Fatal(err) |