amadan
Merge fix/carloku-observe-routes-cleanup-20261001 (023e106c): both reviews accept, CI passed
The observe routes cleanup, reviewed at 51587e00 (Opus accept, Astra accept with fixes; the test notes applied in 023e106c), gated on 51587e00, CI passed on 023e106c. Merged into the launch branch as the first accepted side branch.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
4 files changed,
+95
−2
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.go+7 −2internal/registry/observe_test.go+21 −0
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.go b/internal/registry/observe.go| index 43eb59bb7..9b6c91bd1 100644 |
| --- a/internal/registry/observe.go |
| +++ b/internal/registry/observe.go |
| @@ -41,10 +41,15 @@ func (r *Registry) ObserveRoutes(ctx context.Context) (obs *RouteObservation, er |
| db.Close() |
| return nil, err |
| } |
| - obs = &RouteObservation{db: db, conn: conn} |
| + // built, not the named result: "return nil, err" clears the named |
| + // result before the deferred cleanup runs, and the cleanup has to |
| + // close what was built. Closing through the cleared result was a nil |
| + // dereference: any failure after the connection opened was a panic. |
| + built := &RouteObservation{db: db, conn: conn} |
| + obs = built |
| defer func() { |
| if err != nil { |
| - obs.Close() |
| + built.Close() |
| obs = nil |
| } |
| }() |
diff --git a/internal/registry/observe_test.go b/internal/registry/observe_test.go| index dff144a73..b2513248c 100644 |
| --- a/internal/registry/observe_test.go |
| +++ b/internal/registry/observe_test.go |
| @@ -215,3 +215,24 @@ func TestObserveRoutesHelperProcess(t *testing.T) { |
| t.Fatal(err) |
| } |
| } |
| + |
| +// 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) |
| + } |
| + obs, err := reg.ObserveRoutes(context.Background()) |
| + if err == nil || obs != nil { |
| + t.Fatalf("ObserveRoutes over a registry with no routes table: obs=%v err=%v, want an error and no observation", obs, err) |
| + } |
| +} |