Skip to content

fix(worker): coordinate pending starts with invocation handshakes - #711

Draft
jumski wants to merge 1 commit into
mainfrom
696-worker-startup
Draft

jumski wants to merge 1 commit into
mainfrom
696-worker-startup

Conversation

@jumski

@jumski jumski commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Prevent duplicate HTTP worker starts while initialization remains pending. Debounce stays a retry throttle, not a readiness signal.

ensure_workers / authenticated manual or local request
  reserve_worker_invocation(function_name)
    lock registration; skip healthy or unexpired pending starts
    reserve invocation_id with a 60-second database-clock deadline
  dispatch { invocation_id }
    reconcile duplicate deliveries / acknowledge an existing healthy worker
    initialize and compile
    accept_worker_invocation
      atomically consume the current invocation and register the ready worker
    on failure: dismiss only the matching pending invocation

Include bounded startup recovery, the generated migration and types, protocol regressions, and SQL-first rollout documentation. A separate formatting-only commit fixes five array-spacing violations required by the pre-push hook; Atlas confirms no schema drift. Process workers remain outside HTTP scheduling. Lifetime ownership and stale-worker fencing remain in #697.

Fixes #696.

Evidence

  • Before: cron could dispatch another start after the 6-second debounce while the first worker still initialized. A request receipt did not prove readiness.
    After: shared SQL reservations prevent another pending start; real-SQL tests cover independent runtimes, duplicates, expiry, superseded requests, lost acknowledgements, and manual/local compatibility.
  • Before: review regressions exposed a replacement with an aborted signal after watchdog recovery and a delayed reconciliation that stopped a healthy worker.
    After: both regressions pass and prove actual task completion after recovery.
  • Passed: migration verification and migration-backed generated-type checks; full pgTAP (321 files, 1,702 assertions, including upgrade fixtures); integration (81 tests); Edge Function E2E (13 tests); portable-runtime E2E (14 tests across Supabase, Node, and Bun).
  • Passed: worker unit tests (279 tests plus 17 steps), typecheck/type tests/lint, Node/Bun package smoke checks, and website build/internal links. Standards and Spec reviews, correction rechecks, and generated-migration reviews report zero remaining blockers.
  • Push gate passed: all 28 Nx tasks completed successfully, including core SQL lint, core/client Vitest type checks, and all affected package pre-push targets. Changeset validation passed. Known baseline issue outside this gate: four type errors remain in unchanged demo sources; its focused production build passes.

Merge Danger

Door: two-way

The database migration adds nullable invocation fields and startup functions without deleting data or changing existing worker registrations. Worker rollback remains compatible; database rollback needs manual SQL because no automatic down migration exists.

Apply the SQL migration before the worker update. Mixed-version rollout remains compatible, but old workers do not enforce the new handshake. Lost or hung startup recovery can take up to 60 seconds.

Blast Radius: startup

Changes affect cron, authenticated manual, and local HTTP worker startup. Authentication remains enforced. Hosted runtimes cannot be forcibly evicted; lifetime ownership and task fencing are intentionally outside this PR.

@changeset-bot

changeset-bot Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 61c75a5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@pgflow/core Patch
@pgflow/edge-worker Patch
pgflow Patch
@pgflow/client Patch
@pgflow/dsl Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

jumski commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@nx-cloud

nx-cloud Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

View your CI Pipeline Execution ↗ for commit 61c75a5

Command Status Duration Result
nx run-many -t build --projects=dsl,core,cli ✅ Succeeded 5s View ↗

💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗


☁️ Nx Cloud last updated this comment at 2026-11-02 12:41:49 UTC

invocation_id uuid,
invocation_expires_at timestamptz,
invocation_status text check (invocation_status in ('pending', 'ready', 'failed')),
invocation_worker_id uuid,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we allow it to be null? shouldn't it be an fkey?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null is intentional: it is set only when a handshake reaches ready. Left as a soft pointer on purpose — pgflow never deletes worker rows (only deprecates), so an FK would add migration churn without behavior change. Can revisit if workers ever gain cleanup.

Comment on lines +11 to +14
select wf.* into registration
from pgflow.worker_functions as wf
where wf.function_name = reserve_worker_invocation.function_name
for update;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we order here so the locking is deterministic?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a point lookup on the primary key, so at most one row locks here. The multi-function lock order is pinned in ensure_workers, whose reservations CTE locks in order by wf.function_name.

Comment on lines +16 to +18
if not found or not registration.enabled or registration.start_mode <> 'http' then
return null;
end if;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not put the filters like enabled and start_mode into the select wf.* into registrations above? i think this whole function could be language sql/declarative if we just filter appropriate rows? do we need to throw any errors? null (empty result set) can be declaratively returned, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — now language sql with a locked eligible CTE and one update ... returning. No error paths existed to preserve: null is the only non-reserve result. One real trap surfaced while converting: not (invocation_status = 'pending' and ...) is NULL for a fresh registration, which the plpgsql IF treated as false but WHERE semantics filter out — predicates are now NULL-safe (is distinct from). The full ensure_workers pgTAP suite covers it.

-- Validate every HTTP delivery, including deliveries to surviving runtimes.
-- A receipt never clears pending state. An existing healthy worker may finish
-- the handshake without registering a new worker or starting another loop.
create or replace function pgflow.reconcile_worker_invocation(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to make this a declarative, language sql function by leveraging cte's etc

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kept plpgsql here and in accept_worker_invocation. Both are multi-exit state machines (pending/ready/rejected plus a conditional update and an idempotent early return); the CTE rewrite of reserve was clean, but these read clearer imperatively. reserve_worker_invocation — the one you also flagged — is now declarative.

with
-- Detect environment
env as (
env as materialized (

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we need to materialize?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PG12+ inlines plain CTEs. materialized pins is_local() and the Vault reads to one evaluation per statement (consistent environment detection, no repeated decryption); the old code relied on pre-PG12 always-materialized CTEs.

Comment on lines -42 to -101
debounce_passed as (
select wf.function_name, wf.debounce
from http_worker_functions as wf
where wf.last_invoked_at is null
or wf.last_invoked_at < now() - wf.debounce
),

-- Find functions that have at least one alive worker
functions_with_alive_workers as (
select distinct w.function_name
from pgflow.workers as w
inner join debounce_passed as dp on w.function_name = dp.function_name
where w.stopped_at is null
and w.deprecated_at is null
and w.last_heartbeat_at > now() - dp.debounce
),

-- Determine which functions should be invoked
-- Local mode: all enabled functions (bypass debounce AND alive workers check)
-- Production mode: only functions that pass debounce AND have no alive workers
functions_to_invoke as (
select wf.function_name
from http_worker_functions as wf
where pgflow.is_local() = true -- Local: all enabled HTTP functions
or (
-- Production: debounce + no alive workers
wf.function_name in (select dp.function_name from debounce_passed as dp)
and wf.function_name not in (select faw.function_name from functions_with_alive_workers as faw)
)
),

-- Make HTTP requests and capture request_ids
http_requests as (
select
fti.function_name,
net.http_post(
url => c.base_url || '/' || fti.function_name,
headers => case
when e.is_local then '{}'::jsonb
else jsonb_build_object(
'Content-Type', 'application/json',
'Authorization', 'Bearer ' || c.service_role_key
)
end,
body => '{}'::jsonb
) as request_id
from functions_to_invoke as fti
cross join credentials as c
cross join env as e
where c.base_url is not null
and (e.is_local or c.service_role_key is not null)
),

-- Update last_invoked_at for invoked functions
updated as (
update pgflow.worker_functions as wf
set last_invoked_at = clock_timestamp()
from http_requests as hr
where wf.function_name = hr.function_name
returning wf.function_name

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why all this is removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The debounce/alive-worker CTE chain moved into reserve_worker_invocation, which manual startup now shares — the comments moved with the logic. Restored concise per-CTE comments in ensure_workers (env/credentials, one reservation per function, async delivery).

msg_ids BIGINT[],
vt_offsets INTEGER[]
msg_ids BIGINT [],
vt_offsets INTEGER []

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why those changes? is it core:fix-sql? we dont want to introduce like a huge diff/migration due to styling/lint fixes... im pretty sure it is somewhat misconfigured or version-mismatched or wrongly invoked sqruff that causes those...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You were right — version mismatch. Local sqruff 0.39.0 (mise pipx) flags bigint[] in function signatures; CI's setup-sqruff action installs latest (0.44.0), which instead flags the spaced form, so the style commit would have failed CI lint. Dropped 53dcb9a, reset the branch to the compact form, pre-push green under 0.44.0. Upgrading the local sqruff install is the durable fix.

Comment on lines +46 to +59
if (invocationId) {
const accept = () => withStartupTimeout(this.sql, (sql) => sql<WorkerRow[]>`
SELECT * FROM pgflow.accept_worker_invocation(
${edgeFunctionName}, ${invocationId}::uuid, ${workerId}::uuid,
${queueName}, ${pgflowVersion}
)
`, invocationId);
// A connection can fail after COMMIT. Repeat the idempotent operation
// once to recover the same worker, never to authorize another runtime.
const [worker] = await accept().catch(() => accept());
if (!worker) throw new WorkerInvocationRejectedError();
return worker;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it good to have a separate query? shouldn't this be baked into start_worker or somethign that we already ccall?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch replaces the plain INSERT inside onWorkerStarted — the start call we already make — so it is one roundtrip, not an extra query: consume + register atomically. Baking it into ensure_flow_compiled would tie handshake acceptance to flow compilation and change that function's contract.

routes.map(({ stepSlug, queueName }) => ({ stepSlug, queueName })) as unknown as Json
);
const [result] = await this.sql<{ result: EnsureFlowCompiledResult }[]>`
const [result] = await withStartupTimeout(this.sql, (sql) => sql<{ result: EnsureFlowCompiledResult }[]>`

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we reliably say that when timeout was triggered, the ensure_flow_compiled have surely rollback?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. ensure_flow_compiled runs as a single statement inside sql.begin: statement_timeout aborts it server-side, postgres.js rolls the transaction back, and if the connection dies the server discards the uncommitted transaction — idle_in_transaction_session_timeout=60s bounds the half-open case. No autonomous transactions (no dblink anywhere in schemas), so nothing can escape the rollback.

@@ -9,7 +9,7 @@ import { Aside, CardGrid, LinkCard } from "@astrojs/starlight/components";

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File-level: should we have a dedicated page explaining this reconcillation and shit? or some worker torubleshooting?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went with a section: added Troubleshooting worker startup to worker-management (startup_state, invocation_error, both 409 reasons, duplicate-live-workers guidance) instead of a new page — the handshake explanation already lives in that page's Startup handshake section.

Debounce measured HTTP dispatch time rather than readiness, so slow initialization let cron enqueue duplicate live workers. Reserve one database-clock invocation per function, then atomically accept its current unexpired token with ready-worker registration instead of extending debounce.

Use a 60-second pending deadline, idempotent acknowledgement, safe dismissal, and bounded runtime/SQL startup recovery. Authenticated manual and local requests share reservations; process workers stay outside HTTP scheduling. Include the generated migration, types, regression coverage, and SQL-first mixed-version rollout guidance. Lifetime ownership and task fencing remain in #697.

Review feedback: reserve_worker_invocation is now a declarative language sql function. Its eligibility predicates are NULL-safe because a fresh registration has no invocation state; the previous `not (status = 'pending' and ...)` form filtered fresh rows out under WHERE semantics while the plpgsql IF treated the NULL as false. reconcile_worker_invocation and accept_worker_invocation stay plpgsql: they are multi-exit state machines whose early returns read clearer imperatively. invocation_worker_id stays a soft pointer without a foreign key: pgflow never deletes worker rows, so the constraint would add migration churn without behavior. A startup troubleshooting section joins the worker management docs.

The four baseline array-spacing lint failures were a local sqruff 0.39.0 defect in function signatures; CI installs a current sqruff that passes the original compact form, so no reformat ships and the earlier style commit was dropped.

Validation: full pgTAP (1702 assertions), the real-SQL worker-invocation integration file (11 tests), Edge Function E2E (13 tests), portable-runtime E2E (14 tests), migration verification, generated-type checks, and the website build all pass, with the full 81-test integration suite from the pre-review delivery.

Fixes #696
@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

🔍 Preview Deployment: Website

✅ Deployment successful!

🔗 Preview URL: https://pr-711.pgflow.pages.dev

📝 Details:

  • Branch: 696-worker-startup
  • Commit: b1c9109597f2798d88bcae6fc8787f0bce388073
  • View Logs

_Last updated: _

This branch was successfully deployed

1 active deployment
preview — 61c75a5e Deployed Nov 2, 2026 by jumski via deploy-website #3425
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.

Prevent duplicate pending worker starts with invocation IDs and acknowledgements

1 participant