Files
Ruben Fiszel 22eadab67d perf: resolve the worker external IP in the background (#10697)
* perf: resolve the worker external IP in the background

`run_workers` awaited `external_ip::get_ip()` — an HTTPS GET to
hub.windmill.dev — before spawning any worker, so every worker process paid
that round trip before its first job pull. Measured on a CE debug build it was
120-450 ms of a ~200-500 ms startup, and behind a firewall the call does not
fail fast: it burns its whole 5 s connect timeout, on every process start. That
cost is per-job under EXIT_AFTER_N_JOBS.

The value is informational (it is only written to `worker_ping.ip`, which the
workers list displays so users can whitelist the address), so nothing needs to
wait on it. It now resolves into a process-wide cache off the startup path, and
`WORKER_EXTERNAL_IP` supplies it explicitly for deployments that know their
egress address or have no egress at all.

Until it resolves the ping carries no IP, which `insert_ping_query` now
COALESCEs so a reclaimed row keeps the address the previous process wrote
instead of being blanked. The main loop reports the IP as soon as it lands
rather than on the next periodic tick, so a short-lived process still records
it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: keep unknown worker IPs out of the whitelist alert

Review follow-ups:

- `WhitelistIp` filtered only the `'unretrievable IP'` sentinel, so the `'NO IP'`
  one a pending or failed lookup now leaves in the row would be offered as an
  address to whitelist. It filters both.
- Register `WORKER_EXTERNAL_IP` in `ENV_SETTINGS` so operators can confirm from
  the instance settings view that it took effect.
- The worker tracked whether it had reported the IP by re-reading the cache
  after each ping rather than remembering what the ping carried, so a lookup
  landing mid-ping marked it reported without it reaching the row. The value is
  read once and threaded through `insert_ping` / `update_worker_ping_full`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: report a sentinel IP once the lookup has definitively failed

Keeping the previous process's address on a reclaimed `worker_ping` row is right
while the lookup is still in flight, but not once it has failed: the row would
advertise an address nothing has confirmed, and the whitelist alert would offer
it. A failed lookup now reports `UNKNOWN_IP`, leaving NULL to mean "in flight".

`WORKER_EXTERNAL_IP` is rejected when longer than the `varchar(50)` column
rather than panicking the worker on its initial ping, which is a hard failure.

Adds the regression guard for the `ON CONFLICT` semantics: reverting to
`ip = EXCLUDED.ip` would compile and blank every reclaimed row.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: keep the agent initial ping acceptable to older servers

An agent worker routinely runs against a server of a different version, and one
predating the background lookup rejects an initial ping carrying no IP — which
`run_worker` turns into a panic, so a newly upgraded agent would crash-loop
against it. The not-resolved-yet case goes over the wire as the sentinel
instead, and the server maps it back so a reclaimed row still keeps its address
while resolution is pending.

Also documents `ip` as the one conditional exception to `insert_ping_query`'s
"only `started_at` and `jobs_executed` survive a restart", and adds
`WORKER_EXTERNAL_IP` to the README env-var table.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: deliver the resolved IP to servers that only take it at registration

A server predating the background lookup applies `ip` from the initial ping
only, and ignores it on the periodic ones. An agent registering before its
lookup resolves would therefore keep the sentinel forever on such a server,
where it used to report its real address. It registers a second time once the
address is known, skipping that when the address is still unknown, when the
server is reached over SQL and needs no second registration, or once a job has
run, since registering clears the row's current job.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: re-register the resolved IP even after a job has run

Gating the second registration on "this process has not run a job yet" meant an
agent that pulled queued work before its lookup resolved never delivered the
address to a server that only takes one at registration. No job of the worker is
in flight where that runs, so the gate bought nothing beyond the last job's id,
which the next job refills.

Documents the two cases where WORKER_EXTERNAL_IP stops being an optimisation and
becomes the only way to report an address: an agent against such a server, and a
process shorter-lived than the lookup.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* revert: drop the WORKER_EXTERNAL_IP escape hatch

Supplying the address by hand skips the hub lookup, which is not something to
make easy. Resolving it in the background is what keeps it off the startup path;
opting out of it is a separate decision this does not need to take.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: distinguish an IP never established from one that could not be retrieved

`NO IP` was doing double duty: the column default for a row whose lookup has not
resolved, and the marker for one that failed. An operator reading the workers
list could not tell "not resolved yet" from "this instance cannot reach the
hub", and the latter is the actionable one. A failed lookup now reports
`unretrievable IP`, which is also what it reported before the lookup moved off
the startup path.

That leaves `NO IP` meaning only "no address established", which is what an
agent sends while its lookup is in flight and what the server maps back to
"unresolved" — so the wire sentinel no longer collides with the failure marker,
and an agent delivers the failure to a server that only reads an IP at
registration.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-14 14:27:21 +02:00

45 lines
1.3 KiB
Rust

use sqlx::{Pool, Postgres};
use windmill_common::{external_ip::UNKNOWN_IP, worker::insert_ping_query};
async fn insert_ping(db: &Pool<Postgres>, worker: &str, ip: Option<&str>) -> anyhow::Result<()> {
insert_ping_query(
"test-instance",
worker,
"default",
ip,
&[],
None,
None,
"test",
None,
None,
None,
false,
db,
)
.await?;
Ok(())
}
/// The external IP resolves in the background, so the initial ping often has none yet. That must
/// not blank the address a previous process wrote to the row this one reclaims — worker names are
/// stable across restarts under EXIT_AFTER_N_JOBS.
#[sqlx::test]
async fn unresolved_ip_keeps_the_reclaimed_rows_address(db: Pool<Postgres>) -> anyhow::Result<()> {
insert_ping(&db, "wk-reclaimed", Some("1.2.3.4")).await?;
insert_ping(&db, "wk-reclaimed", None).await?;
let ip: String = sqlx::query_scalar("SELECT ip FROM worker_ping WHERE worker = $1")
.bind("wk-reclaimed")
.fetch_one(&db)
.await?;
assert_eq!(ip, "1.2.3.4");
insert_ping(&db, "wk-fresh", None).await?;
let ip: String = sqlx::query_scalar("SELECT ip FROM worker_ping WHERE worker = $1")
.bind("wk-fresh")
.fetch_one(&db)
.await?;
assert_eq!(ip, UNKNOWN_IP);
Ok(())
}