[Areson]

Stack: PR 2 of 5. Stacked on #187. Review only this PR's commits, 4dbb925 and 9a11cc4.

Why

config.monitor.timeout-connect (default 10s) is parsed but never reaches the MySQL driver's DSN, so the driver dials with no timeout. When a host drops packets, each new connection waits for the kernel's TCP SYN retries, about 130 s on Linux, instead of failing fast.

What

The connection factory sets the DSN timeout param from timeout-connect.

How

• An empty value uses the default (10s).
• 0 disables the timeout, which was the old behavior.
• An invalid or negative value fails config validation: mysql.timeout-connect at server boot and monitor.timeout-connect when the monitor loads. Make keeps the same check as a backstop.
• Every Blip MySQL connection (monitor, plan loader, heartbeat) comes from this factory. The hotswap credential reload keeps the param.

Risk

Medium. Every MySQL connection now has a 10 s dial timeout by default. A server that takes longer than that to accept a connection now fails where it used to wait. Set timeout-connect: 0 to get the old behavior.

Testing

A local benchmark against MySQL 8.0 servers put a half-open network partition between Blip and one server.

• Connection attempts failed after 10 s instead of hanging for about 130 s.
• After the network came back, recovery dropped from 20–24 s to 9–10 s.

Bigger picture

Part of a stack of 5 fixes found by the same benchmark. This one doesn't depend on #187's code. It sits on top only to keep the stack linear.

🤖 Generated with Claude Code

[zyin-squareup]

🤖 This review was generated by an AI agent on behalf of @zyin-squareup.

Summary: LGTM. timeout-connect now actually reaches the go-sql-driver DSN. I checked:

• dbconn/factory.go:98 / :148: parsing happens after my.cnf defaults are applied. Empty falls back to DEFAULT_MONITOR_TIMEOUT_CONNECT, which covers callers that skip defaults, like the global plan-table path in plan/loader.go:106. 0 leaves out the param, and negative or invalid values return an error.
• Make is the only sql.Open path for monitor, plan loader, and heartbeat connections. Repo.ReloadDSN (dbconn/reload_password.go:39-72) does ParseDSN → FormatDSN, so Timeout survives a credential hotswap. TestConnectTimeoutReloadDSN covers this.
• In go-sql-driver v1.8.1, timeout only sets net.Dialer.Timeout. It does not limit queries on an established connection, which matches the updated docs.
• I ran go test ./dbconn -run TestConnectTimeout -v locally and all tests pass. TestConnectTimeoutBoundsDial failed after 1.0s with dial tcp 10.255.255.1:3306: i/o timeout.

Non-blocking:

1. Upgrade behavior for malformed values. A config that used to have a bad timeout-connect (for example "10" with no unit) was silently ignored before. Now Make returns an error, so that monitor won't start or connect. Nothing in config.go validates this field, so the error only appears per monitor at runtime, not at boot. Consider validating it in the config validation path so it fails fast, and/or adding a release note next to the "default is now 10s" note.
2. Network-dependent test. TestConnectTimeoutBoundsDial dials 10.255.255.1 during a normal go test ./... and is skipped only with -short. The 20s context backstop keeps it from hanging, and the pass condition holds whether the network drops or rejects the packets, so this is fine. Just be aware it depends on the CI runner's network.

[zyin-squareup]

🤖 This review was generated by an AI agent on behalf of @zyin-squareup.

Re-approving after the rebase. The new head 8c97089 has the same content as 405eed5, which I approved earlier: dbconn/factory.go, dbconn/factory_test.go and docs/content/config/config-file.md have the same blob SHAs (278c268, 7200eba, ffb62ec). The only changes come from the updated #187 base, which touches monitor/sink/status, not dbconn. My earlier review still applies, including the two non-blocking notes: malformed timeout-connect values now fail per monitor in Make instead of at config validation, and TestConnectTimeoutBoundsDial depends on the network.

CI was still queued when I checked.

[zyin-squareup]

🤖 This review was generated by an AI agent on behalf of @zyin-squareup.

Re-approving after the rebase onto #187's new head (54b2026). The new head 1aff2f2 has the same content in this PR as 8c97089 and 405eed5, which I approved earlier. dbconn/factory.go, dbconn/factory_test.go and docs/content/config/config-file.md have the same blob SHAs (278c268, 7200eba, ffb62ec). The neighbouring code I checked last time is also unchanged: dbconn/reload_password.go, plan/loader.go and go.sum. The rest of the compare diff comes from the updated #187 base (monitor/sink/status), not from this commit. CI (build, Semgrep, zizmor, DCO) is green on 1aff2f2.

My first review still applies, including its two non-blocking notes:

1. A malformed timeout-connect value now fails per monitor in Make instead of at config validation.
2. TestConnectTimeoutBoundsDial depends on the network.

[zyin-squareup]

🤖 This review was generated by an AI agent on behalf of @zyin-squareup.

Re-approving after the rebase onto #187's new head (371659c). This PR's content at 4dbb925 is the same as 1aff2f2, which I approved earlier. dbconn/factory.go, dbconn/factory_test.go and docs/content/config/config-file.md have the same blob SHAs (278c268, 7200eba, ffb62ec). The neighbouring code is also unchanged: dbconn/reload_password.go, plan/loader.go, config.go, go.mod and go.sum. git diff --stat 371659c 4dbb925 shows only those three files. The new base commit 371659c changes only counter docs (docs/content/api/status.md, monitor/monitor.go, status/counters.go) and doesn't touch dbconn.

I didn't rerun the tests locally this round because the code is the same as what I tested before. On 4dbb925, CI build (both jobs), zizmor and DCO pass. Semgrep was still queued when I checked.

My first review still applies, including its two non-blocking notes:

1. A malformed timeout-connect value now fails per monitor in Make instead of at config validation.
2. TestConnectTimeoutBoundsDial depends on the network.

[Areson]

🤖 Thanks for the two non-blocking notes.

1. Malformed timeout-connect: 9a11cc4 validates it when the config loads. ConfigMySQL.Validate checks mysql.timeout-connect, so a bad default fails at server boot. ConfigMonitor.Validate checks monitor.timeout-connect, so a bad monitor value fails the load like any other invalid field. Empty and zero are still valid, and Make keeps its check as a backstop. The release notes now have an Unreleased entry for the new 10s default and the stricter validation.
2. TestConnectTimeoutBoundsDial: I left it as is. It passes whether the runner's network drops the packets or rejects them, and the 20s context backstop keeps it from hanging, so it holds on any network.

[zyin-squareup]

🤖 This review was generated by an AI agent on behalf of @zyin-squareup.

Re-reviewed at 9a11cc4. The first commit, 4dbb925, is unchanged from my last approval: git diff 4dbb925 9a11cc4 touches only config.go, config_test.go, monitor/loader_test.go and the release notes. So this review covers just the new validation commit.

My earlier notes:

1. Malformed timeout-connect only failed per monitor in Make. Fixed and pinned.
   • ConfigMySQL.Validate (config.go:919) runs from Config.Validate at boot. That happens after InterpolateEnvVars (server/server.go:172-174).
   • ConfigMonitor.Validate (config.go:478) runs from Loader.save after defaults and both interpolation passes (monitor/loader.go:426-432). Every load source goes through save: plugin, config file, monitor files and AWS.
   • config.plans.monitor is covered too, through ConfigPlans.Validate → ConfigMonitor.Validate.
   • Empty values still pass, so monitors that skip defaults keep falling back to the factory's 10s default. Zero is still valid.
   • The release note covers the stricter validation.
2. TestConnectTimeoutBoundsDial depends on the network. The author chose to keep it, and I agree. The 20s context backstop stops it hanging, and the pass condition holds whether the network drops or rejects the packets.

Pin check (from the assertions, not mutation runs): the strings invalid config.mysql.timeout-connect and invalid config.monitor.timeout-connect only come from the two new validTimeoutConnect calls. If either call is removed, TestValidateTimeoutConnect fails. If the monitor check is removed, TestLoaderInvalidTimeoutConnect fails because Load no longer returns that error. I couldn't run actual mutations because the scratch checkout is read-only for edits.

Validation: I ran go test . ./dbconn ./monitor -run 'TestValidateTimeoutConnect|TestApplyDefaultConfig|TestConnectTimeout|TestLoaderInvalidTimeoutConnect' -count=1. It exited 0 and all 6 tests passed; TestConnectTimeoutBoundsDial took 1.00s. CI build and DCO are green on 9a11cc4.

Non-blocking:

• The PR description's "How" section still says an invalid value "is a config error from Make". It's now rejected when the config loads, with Make as a backstop. Consider updating that line.