Skip to content

fix(orchestrator): detect crashed orchestrators in seconds, not minutes - #108

Open
psteinroe wants to merge 1 commit into
mainfrom
fix/crash-detection
Open

psteinroe wants to merge 1 commit into
mainfrom
fix/crash-detection

Conversation

@psteinroe

Copy link
Copy Markdown
Owner

What

Shorter liveness defaults, so a crashed orchestrator's executions are released in about 35 s instead of 5–9 min. Heartbeats can no longer be delayed by handlers that hold every pool connection.

  • Defaults (src/orchestrator.ts): heartbeat every 5 s (was 30 s). An orchestrator is stale after 30 s without a heartbeat (was 300 s). Recovery runs after every heartbeat (was every 8th, about 240 s) and once at startup. Worst case: the last heartbeat comes just before the crash, the row is stale 30 s later, and the next heartbeat of any live orchestrator recovers it within 5 s. A process that comes back after a crash also recovers its predecessor at startup. No new options.
  • Own connection for liveness: registration, heartbeats, stale recovery and shutdown run on one connection per orchestrator, created from the conductor's pool options by DatabaseClient.withOwnConnection(). postgres.js does the same for its listen connection (it accepts its parsed options as they are). Pool saturation no longer delays heartbeats.
  • Outage guard: after a failed or slow heartbeat (one that takes longer than the interval), an orchestrator waits the stale age before it recovers others. Without this, a 30 s stale age would turn every database outage over 30 s into a cascade: the first orchestrator to reconnect would recover all the others, which may still be in postgres.js reconnect backoff (up to 20 s), and they would stop.
  • _private_remove_orchestrators() returns early when it deletes no orchestrator. This is not settlement SQL, so it does not overlap perf(settlement): plan settlement for one partition #103.

Why / numbers

[reported] Pool saturation is real. I wrote the test first: a handler holds the pool's only connection in a transaction. Before the fix, last_heartbeat_at did not move for 60 fake seconds, so the orchestrator would have been recovered while it was still running. With the own connection, heartbeats keep advancing.

Cost at 50 orchestrators with 52 queue partitions, measured with EXPLAIN (ANALYZE, BUFFERS):

  • Heartbeat: a PK update on a 50-row table. 0.16 ms execution, 9 buffers. 10 per second at 50 orchestrators.
  • Recovery when nobody is stale: the delete seq-scans 50 rows in 0.12 ms. Before the early return, the release update ... where locked_by = any(null) still visited every partition: 1.9 ms execution, ~29 ms first-call planning, and ~1.3–3.4 ms per call in a loop. With the early return a call costs ~0.3–0.4 ms round trip, against ~0.2–0.3 ms for select 1. Running recovery after every heartbeat therefore costs about 1 ms of DB time per second across 50 orchestrators, and it no longer grows with the number of queues.
  • Each orchestrator keeps one extra idle connection, so 50 orchestrators add 50 connections.

Lost-claim grace (#93): already gone. #93 moved the orphan sweep into the claim statement, so nothing there depends on the heartbeat interval. CLEANUP_TIMEOUT_MS (10 s) and the final flush deadline (10 s) stay below the stale age.

Tests

New tests in orchestrator-cleanup.test.ts:

  • recovery at startup of an orchestrator that crashed before this one started
  • a live orchestrator recovers a crashed one within a heartbeat and a poll of the stale age
  • after missing heartbeats, an orchestrator waits the stale age before recovering others (this test fails without the guard)
  • heartbeats keep running while handlers hold every pool connection (this test failed before the fix)

Liveness now runs on its own connection and keeps real time, so tests that made orchestrators stale with the test clock now move last_heartbeat_at back instead. The own connection opens on a timer, so fake-timer tests use a startWithFakeTimers helper. Tests that inject faults into orchestrator statements on conductor.db stub withOwnConnection to return it. The fixture drops test databases WITH (FORCE), so one failing test that leaves an orchestrator connected does not make the following tests fail.

just lint, just format, pnpm typecheck and the full pnpm test pass (618/618). One earlier full run had a single failure in wait-deadlines.test.ts > waitForResult with a completion settled after the deadline > times out. It passed 3 of 3 times on its own and in the next full run. It looks like a timing flake under parallel load, and I found no link to this change.

Follow-ups / notes

  • A handler that blocks the event loop for 30 s now makes its orchestrator look stale; before, that took 5 min. This is documented in docs/scaling/horizontal.md (Crash Recovery).
  • Startup recovery is not guarded: an orchestrator that starts right after a database outage can recover peers that have not reconnected yet. Startup recovery is what releases a crashed process's work as soon as it restarts, so I kept it.
  • A falsely recovered orchestrator still stops instead of re-registering. That is out of scope here.
  • idx_orchestrators_heartbeat makes every heartbeat a non-HOT update, and the planner seq-scans the table anyway. Dropping the index would be a cheap follow-up.

Heartbeat every 5s and treat an orchestrator as stale after 30s, recovering
after every heartbeat and once at startup, so a crashed process's executions
are released within about 35s instead of 5-9 minutes.

Heartbeats, registration, recovery and shutdown run on a connection of the
orchestrator's own, so handlers holding every pool connection no longer delay
heartbeats past the stale age. After a failed or slow heartbeat an
orchestrator waits the stale age before recovering others, so a database
outage does not make the first one to reconnect recover all the rest.
_private_remove_orchestrators() returns early when it removes no one.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant