v3: rewrite core driver with truthful optional-interface wrapping, lean dependencies, stdlib tests

[daniel-garcia]

Summary

Full rewrite of the hotload driver as github.com/infobloxopen/hotload/v3. The driver name, DSN format (strategy://driver/path?forceKill=...), registration functions, Strategy interface, and graceful/forceKill semantics are unchanged — for most applications the upgrade is the import-path change. See MIGRATION.md.

Truthful driver capabilities

database/sql discovers capabilities by type-asserting optional interfaces on the driver conn. v1 implemented a fixed set regardless of the underlying driver (papering over gaps with driver.ErrSkip) and never implemented ConnPrepareContext/Pinger or wrapped driver.Stmt. v3 wraps conns and stmts so an optional interface exists iff the underlying driver supports it:

• Single-method piece types are composed into generated combination structs (internal/gen, run via go generate, CI fails on diff): 16 conn combos over {ExecerContext, QueryerContext, Pinger, NamedValueChecker} and 16 stmt combos over {StmtExecContext, StmtQueryContext, ColumnConverter, NamedValueChecker}.
• Legacy Execer/Queryer collapse into their context flavors, with database/sql's legacy fallback replicated internally; the wrapper never advertises the legacy interfaces.
• ConnPrepareContext, ConnBeginTx, SessionResetter, Validator are always implemented, replicating the stdlib fallback exactly (hotload's drain/evict machinery needs the latter two).
• Statements are wrapped for the first time, so prepared-statement traffic now counts in transaction_sql_stmts.

Lifecycle rewrite

• chanGroup is replaced by a group/generation model: one generation owns the cancel context, conn set, and drained flag for one DSN value, so a config change is a pointer swap. No driver call, hook, or strategy call happens under any lock — removing the nested-lock deadlock class (573e701) and its 1ms-sleep workaround.
• teivah/onecontext replaced with a context.AfterFunc cancellation relay: no goroutine per merge, unregistered synchronously, fixing the leak class behind 06f03af. QueryContext/BeginTx still pass the caller context through unmerged (402e736 semantics preserved).
• forceKill cancels with cause hotload.ErrHotSwap and waits a bounded killWindow (new DSN param, default 100ms) before force-closing, so a driver that ignores cancellation can never wedge change processing. Dials happen outside locks, so a slow dial no longer blocks the run loop.
• hotload implements driver.DriverContext; connectors are refcounted per DSN and DB.Close now tears down the strategy watch and run loop (v1 leaked both). Config errors surface at sql.Open instead of first use.

Dependency diet

Root go.mod requires exactly github.com/fsnotify/fsnotify (CI-enforced via make dep-budget). prometheus, pkg/errors, google/uuid, onecontext, gaugefuncvec, ginkgo/gomega, sqlmock, and lib/pq leave the consumer module graph:

• Metrics are emitted through a zero-dependency hooks API (hotload.RegisterHooks); the new observability/ nested module adapts them to prometheus with v1's exact metric names (registration is now explicit — the loudest item in MIGRATION.md). gaugefuncvec is replaced by a hand-rolled scrape-time collector.
• Integration tests move to a never-imported test/integration nested module (lib/pq + docker-compose, opt-in via env).

k8ssecret strategy (ports #66 onto v3)

A new k8ssecret/ nested module implements the strategy idea from #66: watching a Kubernetes Secret through the API server for deployments that need credentials from another namespace (which Kubernetes cannot mount as a volume). Same DSN shape (k8ssecret://<driver>/<secret>?namespace=<ns>&dsn=<key>), separate go.mod so client-go stays out of the core. Beyond the port, it fixes four defects in the #66 implementation:

• The hotload core passes uri.Path, so the secret name arrives with a leading slash; Add k8ssecret strategy for cross-namespace secret watching #66 used it verbatim and would look up a secret named /myapp-db (its tests called Watch directly with bare names, masking this). Pinned here by an end-to-end test through sql.Open.
• Updates could be lost between the initial Get and the watch start, and while a dropped watch reconnected. Every (re)connect now re-reads the Secret, delivers missed changes, and watches from that read's resource version; Added events (recreated secrets) are handled too.
• Slow subscribers had the newest value dropped when their buffer was full, leaving them permanently stale; delivery now drops the oldest queued value instead.
• Watches were keyed by namespace/name only, so two DSNs reading different data keys of one Secret shared a single value; the data key is now part of the watch identity.

fsnotify strategy fixes

• Watch-closure goroutine leak fixed (one goroutine leaked per CloseWatch in v1); update delivery is now non-blocking (latest value wins), so a dead subscriber can no longer wedge every watcher sharing the strategy.
• Kubernetes ConfigMap ..data symlink swaps are now detected on kqueue platforms (events arrive under the resolved target path; unknown names conservatively resync all watched paths).
• Watch after Close no longer panics.

Tests

All stdlib testing (ginkgo/gomega removed). New suites:

• A capability-masked fake driver (internal/dbfake, views generated alongside the combos) drives interface-truthfulness matrices with positive and negative assertions over every combo, plus database/sql-level dispatch tests proving the pool takes the intended fallback paths.
• Lifecycle: graceful swap/grace period, forceKill mid-exec (asserting ErrHotSwap and exactly-one close), bounded killWindow with a cancellation-ignoring driver, a→b→a, teardown/refcounting, reopen-while-closing race (regression test fails on the unfixed code), change storms under -race with goroutine-leak checks (internal/testutil).
• fsnotify against real files: writes, truncation, atomic rename, remove/recreate recovery, the ConfigMap symlink dance, abandoned subscribers.
• Integration (postgres 10.3 via docker-compose, both forceKill modes): database switchover, password rotation during long operations, transactions spanning changes, context cancellation, prepared statements across changes.

Release notes

RELEASING.md documents the nested-module tag ordering (root v3.0.0 first, then bump observability/go.mod's require and tag observability/v1.0.0) and a consumer smoke check. The replace in observability/go.mod is for in-repo development; consumers resolve the pinned require.

Test plan

• make ci-test (fmt, tidy, generate diff-check, vet, -race tests for all three modules, dep budget)
• Integration suite green twice against postgres 10.3 via lib/pq (make local-integration-tests)
• Goroutine-leak checks on every lifecycle/concurrency test
• Regression test for the reopen-while-closing watch race verified to fail on the pre-fix ordering

[bfabricio]

Given this PR is already rewriting the core, can we revisit the Strategy interface API for v3?

1. Watch(ctx, pth, pathQry) passes two opaque strings. The core has already parsed the hotload DSN, but each strategy has to re-parse / normalize path and query data, then repeat that same identity work in CloseWatch.
2. Collapse Strategy to a single Watch method scoped by context.Context. Removes CloseWatch and Close, eliminates duplicated identity logic in every strategy. Context cancellation can be use for managing lifetime.

I would simplify the strategy boundary to keep the channel, but make the resource parsed and the lifecycle context-scoped:

// Strategy is the plugin interface for hotload.
//
// Watch returns the current value and a channel of subsequent updates.
// The watch lives until ctx is canceled; on cancellation the strategy
// releases its resources and closes the updates channel. Values may
// carry secrets — strategies must not log them.
type Strategy interface {
    Watch(ctx context.Context, loc Location) (value string, updates <-chan string, err error)
}

// Location identifies what to watch, parsed once by the core.
type Location struct {
    Path   string     // URL path (file path, secret name, etcd key, …)
    Params url.Values // query params, hotload-reserved keys removed
}

Why this shape:

1. One method. Teardown is ctx cancellation, the core already owns the per-DSN context.
2. No re-parsing. Location is parsed once.
3. No Close(). Strategy lifetime = process lifetime; per-watch lifetime = ctx. Test isolation comes from constructing fresh strategy instances.

[bfabricio]

Single-method interfaces are one of Go’s most powerful patterns. They are especially expressive when they represent exactly one capability through a single method.

For example, Watch: given a resource, watch it and stream configuration values.

[daniel-garcia]

The parsing would add a small improvement but then you could not just pass the cli/env args directly into it. it pushes that into the caller. for example, net.Dial("tcp", "10.11.12.13:443"), you could pass a parsed version of the destination Addr, but its more convenient to deal with the unparsed representation throughout the code then the small efficiency you may gain. further since strings are immuntable pointers, nothing really gets passaround other than a pointers. once you have a struct here. you are passing a struct val of two pointers with the possiblity of url.Values being a ref, i think. Just doesn't seem worth the effort.

On the watch being the context, yeah I can get onboard.

[daniel-garcia]

The CloseWatch is there to close specific watches but if the Watch returned a closeable, then that would be more ergonomic. Maybe we can have a WatchLocation(ctx, loc) value, Watchable, error

type Watchable interface {
Values() <-string
Close() error
}

wdyt?

[bfabricio]

Yep, fair point on not making the Watch arguments a struct. The net.Dial analogy makes sense to me. keeping the lower-level API string-based is probably more convenient

The main part I still like is collapsing the lifecycle around Watch, and I think your Watchable shape solves the CloseWatch concern cleanly.

Something like this should work:

type Strategy interface {
    Watch(ctx context.Context, path, pathQry string) (Watchable, error)
}

type Watchable interface {
    Values() <-chan string
    Close() error
}

and the usage:

w, err := strategy.Watch(ctx, path, pathQry)
if err != nil {
	return err
}
defer w.Close()

for value := range w.Values() {
	if err := apply(value); err != nil {
		return err
	}
}

[daniel-garcia]

Implemented in efb28ef. Final shape:

type Strategy interface {
    Watch(ctx context.Context, pth, pathQry string) (value string, watch Watchable, err error)
}

type Watchable interface {
    Values() <-chan string
    Close() error // idempotent
}

Notes:

• Kept the synchronous initial value return (one deviation from your last snippet) so a bad config still fails fast at sql.Open instead of surfacing asynchronously.
• Watch lifetime is Watchable.Close or ctx cancellation, whichever comes first — the core passes the group's parent context in, strategies wire cancellation with context.AfterFunc.
• CloseWatch and strategy-level Close are gone, and with them all the path/query re-parsing: the handle is the watch.
• Each Watch call is now an independent subscription. That fixed a latent bug: subscribers used to be keyed by pathQry, so two DSNs differing only in the driver component shared one channel and split updates between them.
• fsnotify now releases its OS watcher when the last watch closes (previously only the strategy-wide Close did).

MIGRATION.md has a new section for custom strategy authors. All modules green under -race, including the postgres integration suite.

[bfabricio]

Would it be possible, or would it make sense, to make this registration explicit?

I understand this follows the database/sql driver pattern, but it feels less ergonomic in this case because strategies are stateful and often require configuration.

In particular, k8ssecret currently has to expose package-level mutable state, such as ClientsetFunc, so callers can influence the instance registered by init(). That makes configuration implicit and order-dependent: users need to import the package for side effects, mutate package globals before opening a DB, and trust that the registered singleton is the one they intended to configure.

I would prefer the blank import to remain available only as a backwards-compatible convenience, while the explicit/configured path is documented as the primary API.

Something like:

// Explicit — testable, no global, errors surface at the call site:
reg := hotload.NewRegistry()
reg.Register("fsnotify", fsnotify.NewStrategy())
reg.Register("k8ssecret", k8ssecret.NewStrategy(
    k8ssecret.WithClientset(cs),       // inject deps here, no package global
))
db := sql.OpenDB(hotload.NewConnector(reg, "k8ssecret://pgx/db?..."))

Instead of:

import _ "…hotload/fsnotify"       // magic, registers itself

[daniel-garcia]

isn't the clientset optional? it should be. we don't support double registration so we have to pick a path and stay on it.

[bfabricio]

I guess my main concern here is less about Clientset specifically being required/optional, and more about how the strategy instance gets registered.

With the blank import path:

import _ "github.com/infobloxopen/hotload/k8ssecret"

the package is effectively doing NewStrategy() inside init() and registering that instance globally.

That means any configuration for that strategy has to happen through package-level mutable state, and callers have to know to set that state before the package is used/opened.

I’m mainly suggesting that for v3 we pick the explicit path the only option. The important part to me is that the configured strategy instance is constructed and registered explicitly by the caller, not hidden behind init()

My previus example was not that good as I double register strategies:

reg := hotload.NewRegistry()
reg.Register("k8ssecret", k8ssecret.NewStrategy())

db := sql.OpenDB(hotload.NewConnector(reg, "k8ssecret://pgx/db?..."))

[bfabricio]

After looking more closely at how things are organized, I’m not sure this would be feasible or make sense. It changes a lot the way this operates.

Maybe this is a cleaner snippet showing what I envision for a more explicit driver declaration and usage:

db := sql.OpenDB(hotload.NewConnector(
    fsnotify.Watch("fsnotify://postgres/tmp/myconfig.txt"),
    hotload.Driver("postgres", &pq.Driver{}),
))