[Areson]

Why

The default MySQL connection factory (dbconn.NewConnFactory) fetches a monitor's credentials once in Make, before it builds the DSN. It gave that first fetch only 500ms.

With AWS Secrets Manager or IAM auth, the first fetch may also have to get AWS credentials. On EKS that can be IRSA (AssumeRoleWithWebIdentity) plus a cross-account AssumeRole, which takes 5 to 11 seconds. So the first Make often failed with operation error Secrets Manager: GetSecretValue, get identity: get credentials: request canceled, context deadline exceeded. Monitors retried, so this was mostly log noise. But loading shared plans from plans.table failed, and Blip exited at startup.

Callers also couldn't tell this timeout from a real failure. aws.Secret.GetSecretPayload formatted the SDK error with %s, so errors.As couldn't find *aws.RequestCanceledError.

What

• Make now waits up to dbconn.DefaultCredentialTimeout (30s) for the first credentials. The new dbconn.WithCredentialTimeout option changes it.
• Errors on the credentials path now wrap their cause. The messages are the same.

How

• The timeout is a ConnFactoryOption, like WithPasswordSecretParser. Make takes no context, and adding one would change the blip.DbFactory interface.
• The clock starts right before the first fetch, so loading the AWS config doesn't use it up.
• Zero disables the timeout. A negative value panics, like time.NewTicker. A factory struct literal gets the default.
• Credential reloads after an auth error (Repo.ReloadDSN) don't change. They already use the driver's connect context.
• %s became %w in GetSecretPayload, GetSecret, and blip.DefaultPasswordSecretParser.
• aws.ConfigFactory.Make dropped the IMDS error when region: auto failed. It now returns it wrapped in a type that keeps the old message.

Risk

Low for healthy setups: they see fewer false startup errors. When AWS is slow or down, Make now blocks up to 30s instead of 500ms. So a monitor stop during startup can take that long, and Blip startup can wait that long for plans.table. The release notes and the monitor-stop section of config-file.md describe this. Error messages didn't change, so callers that match on them still work.

Testing

I put back the 500ms deadline and the %s formatting and checked that the new timeout and errors.As tests fail on that code.

Bigger picture

Two follow-ups are out of scope here:

• aws.ConfigFactory builds a new AWS config, with an empty credentials cache, on every Make. So every monitor repeats the slow first fetch. Caching the config per region would make only the first one slow. Its 200ms IMDS deadline for region: auto is also short.
• Make can't be canceled. A context-aware factory interface would let a monitor stop, or Blip startup, give up the wait.

Generated with Claude Code

[zyin-squareup]

🤖 Review findings - created by zyin-squareup's code review agent - for /pull/199, 3eacb18.

Verdict: ship — 1 finding: 0 blocking, 0 important, 1 nice-to-have, 0 suggestions.

This PR gives dbconn factory.Make 30s instead of 500ms for its first credential fetch, with a WithCredentialTimeout option to change it. It also keeps AWS and JSON error causes by switching %s to %w and wrapping the IMDS region error. The main risk is the longer blocking window during monitor startup and stop. That trade-off is disclosed, and I found nothing that holds a lock or depends on the old bound. The only finding is a docs understatement.

Assumptions & unknowns

• Reviewed head 3eacb18bc19a7416f17b4def16707513c456e4d2 (4 commits) against a fresh origin/main. The head was re-checked before posting.
• Local go build, go vet and go test could not run because this session's sandbox denied them. Validation here means source inspection plus the green CI build job.
• I ran no mutation or pin checks. Test-pinning claims below come from reading the tests.

Nice-to-have

[NICE-TO-HAVE] Monitor-stop docs understate the startup wait when plans come from a table (CONFIRMED)

File:

blip/docs/content/config/config-file.md

Line 209 in 3eacb18

* The monitor is starting. A stop waits for it to make its connections, which calls its credentials function (for example, for an IAM auth token or AWS Secrets Manager password, up to 30 seconds each time by default, while the AWS SDK fetches AWS credentials) or a custom `DbProviderFactory`, and to load its plans, which queries MySQL only if they're in a table ([`plans.table`](#table-1)): up to 5 seconds, connecting included, when MySQL is unreachable or hung.

Failure scenario: A monitor uses a Secrets Manager or IAM auth password and plans.table, and it is stopped during startup. The stop can wait about 30s (the first Make), then another 30s (the Make for plan loading), then 5s (the query). The docs give plan loading "up to 5 seconds, connecting included."
Evidence:

• monitor/monitor.go:301 (m.makeDB()) is followed by monitor/monitor.go:323 (m.planLoader.LoadMonitor(m.cfg, m.dbMaker)). The only stop checks are in the retry-sleep branches.
• LoadMonitor calls dbMaker.Make(mon) again (plan/loader.go:198), which runs a fresh f.credentialContext() fetch (dbconn/factory.go:162).
• The 5s covers only ReadTable's query context (plan/loader.go:490).
• The base had the same wording, but the gap was 500ms. This PR makes it 30s.

Impact is limited. The second slow fetch happens only when the first one was slow but succeeded, and the loader stops monitors in the background with a monitor-stop-timeout event.
Suggested fix: Add to the plan-loading clause: "plus one more credentials call (up to the credential timeout) when plans are in a table."

General suggestions

• The PR already lists two follow-ups that would remove most of this wait: caching aws.ConfigFactory per region (each Make currently rebuilds the credentials cache) and a context-aware DbFactory.

The one thing that could have broken, verified

The longer Make timeout could have blocked something that holds a lock or depends on the old 500ms bound. Verified by inspection that this doesn't happen:

• Make runs in the monitor's runLoop goroutine without runMux, which is taken only to assign m.db (monitor/monitor.go:304-309).
• The loader stops monitors in the background.
• credentialFunc gets the new context only at dbconn/factory.go:162-164. Repo.ReloadDSN still uses the driver's connect context.

Verified correct

• Error messages are unchanged: %s→%w in aws/secret.go:36,52 and blip.go:124 produces the same text, and regionError.Error() returns the old IMDS string exactly. Nothing in the repo matches on or type-asserts these errors.
• Wrapping changes no control flow: the DeadlineExceeded/Canceled checks in monitor/engine.go and status/ handle collector errors, not Make errors. setErr logs with the monitor ID and retries.
• Timeout semantics hold:
  • A zero-value factory{} gets 30s.
  • WithCredentialTimeout(0) maps to the -1 sentinel, which gives a context with no deadline.
  • A negative value panics when the option is built, as documented.
• Credential sources: static, file and my.cnf ignore the context. Only IAM auth and Secrets Manager see the 30s.
• Tests, read but not mutation-checked: TestMakeCredentialTimeoutDefault (700ms provider) fails against a 500ms deadline. TestMakeCredentialTimeoutOption (2s provider, 50ms option) fails if the option is ignored. Both enter through NewConnFactory(...).Make.
• Release notes match the code.
• Categories that don't apply: concurrency beyond the lock check above, security, schemas and migrations.
• CI: build (go build ./... + go test ./...), DCO, Semgrep and zizmor all pass at this head.

This review was generated by Claude Code (claude-opus-5-5). Local tests were not run (sandbox denied go commands); CI build and tests passed.

[zyin-squareup]

🤖 Re-review by zyin-squareup's code review agent - /pull/199, 5665fd7.
Verdict: ship - the only change since the last review is docs, and it fixes the prior finding; 2 new nice-to-haves.
Prior findings: monitor-stop docs understated the wait when plans are in a table - fixed (release note and config-file.md:209 now count the second Make from LoadMonitor)
New findings:

• [NICE-TO-HAVE] Local auto-detect waits up to 30s per probe and isn't mentioned in the docs (monitor/loader.go:668).
  • Trigger: no monitors are configured and top-level aws.iam-auth or aws.password-secret is set, so ApplyDefaults copies it into each localhost probe.
  • Each probe's Make can now block 30s (was 500ms) on a hung source, users × (sockets+1) times, at synchronous startup. The release note names only plans.table.
• [NICE-TO-HAVE] No test covers the region-error wrapping at aws/aws.go:43.
  • TestRegionErrorWrapsIMDSError constructs regionError directly, so changing line 43 to regionError{} still passes go test ./aws/ ..
  • The production code is correct by inspection.
    Validation: go vet is clean and go test ./aws/ . passes. In ./dbconn/, the new tests pass; only TestConnect fails, because it needs MySQL on :33800.
    Pin checks: reverting the default to 500ms and the three %w→%s sites each made a test fail; dropping the region cause failed none. CI is green.

[zyin-squareup]

🤖 Re-review by zyin-squareup's code review agent - /pull/199, bf8c71f.
Verdict: ship - the delta since 5665fd7 is a real entry-point test for the region error plus a release-note line, and both close the open findings.
Prior findings:

• Monitor-stop docs understated the wait when plans are in a table: fixed
• Local auto-detect wait was missing from the docs: fixed
• No test pinned region-error wrapping in ConfigFactory.Make: fixed
  New findings: none
  Validation: go vet clean. go test ./aws/ . and go test -run 'Credential|Timeout' ./dbconn/ pass. Pin check: making Make return regionError{} with the cause dropped fails TestConfigFactoryWrapsRegionError (killed). CI (build, DCO, Semgrep, zizmor) is green.