TLS storage maintenance causes spurious error messages on caddy admin API changes

[jum]

Issue Details

When using a network based storage module for TLS certificates every once in a while a race condition produces a flurry of error messages due to failed network calls with context cancelled messages. A typical example:

{
  "insertId": "19jcz73egp0ed",
  "jsonPayload": {
    "error": "Unable to release lock for caddy/locks/storage_clean: redis: client is closed",
    "logger": "tls",
    "message": "unable to release lock",
    "container": {
      "imageName": "jumager/caddy:develop",
      "id": "0ac09e46fb29343ff57d9ccfea137538b386d8ff634b45139f2495faf346ce0d",
      "name": "/caddy",
      "imageId": "sha256:c7536d4b1e0059446272e1c4835b0e11234554f46ceff910a139ac5457f103be",
      "created": "2026-08-16T16:36:17.364258633Z"
    },
    "storage": "{\"client_type\":\"simple\",\"address\":[\"redacted:6379\"],\"host\":[],\"port\":[],\"db\":\"0\",\"timeout\":\"\",\"username\":\"\",\"password\":\"\",\"sentinel_password\":\"\",\"master_name\":\"\",\"key_prefix\":\"caddy\",\"encryption_key\":\"\",\"compression\":\"\",\"tls_enabled\":false,\"tls_insecure\":false,\"tls_server_certs_pem\":\"\",\"tls_server_certs_path\":\"\",\"route_by_latency\":false,\"route_randomly\":false}",
    "instance": {
      "id": "redacted",
      "zone": "redacted"
    }
  },
  "resource": {
    "type": "gce_instance",
    "labels": {
      "instance_id": "redacted",
      "zone": "europe-west3-a",
      "project_id": "hosting-349701"
    }
  },
  "timestamp": "2026-08-17T22:36:18.338979742Z",
  "severity": "ERROR",
  "logName": "projects/redacted/logs/ngcplogs-docker-driver",
  "receiveTimestamp": "2026-08-17T22:36:19.202080199Z"
}

I did use gemini to analyze the race condition and devise a fix pull request for caddy:

#7954

This also requires a change in certmagic:

caddyserver/certmagic#403

And also in caddy-storage-redis:

pberkel/caddy-storage-redis#43

I am currenty stress testing this fix in production with artificial load on the admin api with constantly changing the server config and thus forcing server reloads. All the PR's are marked as work in progress because I would like to hear some feedback from more experienced caddy developers.

Assistance Disclosure

AI used

If AI was used, describe the extent to which it was used.

Gemini analyzed the code for race conditions and pointed out the issues.

[steadytao]

Sounds reasonable from a glance. Will be back in ~16 hours to take a more thorough look -- @steadytao

[faiyaz032]

small framing thing first, this isn't really a race condition, it's more of a teardown ordering problem. -race won't catch it. probably worth saying since it changes how people look at it.

the bigger one, i think certmagic#403 might be aimed at the wrong mechanism. releaseLock already does this:

// certmagic storage.go:333
err := storage.Unlock(context.WithoutCancel(ctx), lockKey)

cancellation is already stripped there. so the unlock isn't failing from a cancelled context, it's failing because the client got closed underneath it. which means errors.Is(err, context.Canceled) can never match redis: client is closed, only the ctx.Err() != nil half of the guard ever fires, and it fires for an unrelated reason. the error goes away but kind of by accident.

if that's right then #7954 on its own should be enough. once Stop() waits for the cleaner before cancelFunc runs, nothing can close the client mid clean and there's nothing left for the other two prs to suppress. worth checking in your stress test with stock certmagic and stock redis module, that'd settle whether they're needed at all.

last thing, this might be broader than the storage cleaner. wrappedCancel loops over moduleInstances which is a map, so cleanup order is random and storage can get closed before anything still referencing it. the ech publish/rotate goroutine (tls.go:426-453) only exits on t.ctx.Done() and writes to ctx.Storage() in ech.go, same exposure, and #7954 wouldn't cover it.

[steadytao]

As Faiyaz stated. This appears to be Caddy teardown ordering. CertMagic already calls Unlock with context.WithoutCancel; the failure occurs because the storage client has been closed underneath the operation. Caddy should likely cancel and join every TLS-owned storage user, including maintenance and ECH publication/rotation work, before storage cleanup. I would not solve this by suppressing unlock failures, they are legitimate failures.

[jum]

I have updated the pull request to also handle the ech case and to not rely on any changes on the certmagic subsystem. I hope that is more acceptable.

[jum]

I have been stress testing this for a day now, but I still get some spurious error messages regarding closed redis connections from the certmagic module in storage.go:590. So some changes in certmagic or in the way caddy uses certmagic might still be needed.

I also get on these stress tests the message:

setting HTTP/3 Alt-Svc header no port can be announced, specify it explicitly using Server.Port or Server.Addr

This normally disappears on completely restarting caddy.

[faiyaz032]

storage.go:590 is not certmagic, that file is only ~370 lines. more likely the redis module. can you paste the full log line with the logger field?

either way i don't think TLS.Stop() can finish this on its own. the cert cache is a singleton that survives reloads on purpose, and its maintenance runs on its own background context. the admin identity cache is the same, and gets skipped entirely if admin.identity isn't configured. on-demand issuance runs on the handshake context while the old server is still draining. none of those get stopped when the tls app stops, and module cleanup order is just map order anyway, so storage can close before things still using it.

worth looking at shutdown separately too. caddy releases its own certmagic locks after the config context is already torn down.

the alt-svc one looks unrelated. h3 listeners close immediately on reload while the old server keeps draining and logging that per request. quick check: stop the reload loop and see if it goes away without a full restart.

[jum]

Oh, my mistake I worked in storage.go and copy pasted the wrong file name. It is in certmagic/maintain.go:590.

[jum]

I have investigated the alt service error messages a bit more, and there is an additional subsystem in certmagic holding onto the storage provider. The logic certmagic.Cache.maintainAssets() is attached to the shared certificate cache (certCache), which intentionally survives Caddy configuration reloads so TLS handshakes continue without dropping cache state. I think fixing this in certmagic is out of scope of the above referenced PR to caddy.