Skip to content

Add cooperative cancellation and shutdown-drain semantics to the worker framework (#142) - #194

Merged
BigDella merged 1 commit into
Parcel-Protocol:mainfrom
luciusverus-cyber:fix/issues-142-worker-shutdown
Sep 27, 2026
Merged

BigDella merged 1 commit into
Parcel-Protocol:mainfrom
luciusverus-cyber:fix/issues-142-worker-shutdown

Conversation

@luciusverus-cyber

Copy link
Copy Markdown
Contributor

The link problem in this issue

#142 asks contributors to branch off dev and open the PR against dev. That branch does not exist, and the issue that was supposed to create it — #104 [DevOps] Establish and protect the dev integration branch — is closed. So the Contribution Workflow in this issue cannot be followed as written.

This PR therefore targets main, the only branch it can merge into. It is otherwise self-contained: three files, none of them shared with the other open worker work.

What was actually broken

Two things, and the second is why the first mattered.

1. Async handlers were never awaited. runJob invoked registration.handler(...) and discarded the result, then immediately moved the job to succeeded. A promise-returning handler was therefore reported as complete before it had done anything, and its rejection escaped as an unhandled rejection. This is the mechanism behind the issue's "a maintenance job can be marked completed after its side effect was abandoned".

2. Nothing could be cancelled. A handler had no way to learn it should stop, and there was no way to ask what was in flight when a process stopped.

The contract

WorkerContext now carries an AbortSignal spanning the whole framework. The behaviour at shutdown is explicit per state:

state at abort outcome
queued / retrying never started — left exactly as they are, reported as pending. Nothing was abandoned, so the next process re-runs them.
running the attempt was abandoned — moved to retrying, or dead_lettered once the attempt budget is spent, and reported as interrupted. Never succeeded.
succeeded / dead_lettered settled before the abort — untouched, counted in settled.
  • drainDueJobs() runs sync handlers inline, tracks async ones, and reports the rest as inFlight instead of pretending they finished. drain() awaits a final outcome, including work an earlier synchronous drain left running.
  • shutdown() aborts, waits for in-flight handlers up to a timeout, and classifies from the state captured at abort time — a handler that ignores its signal and never settles is still running with an unknown side effect, so it is reported as interrupted rather than pending.
  • Once cancelling, neither drainDueJobs nor processJob starts new work, so nothing is re-entered after a shutdown.
  • Cancellation is decided by the framework's own signal and nothing else. A handler that throws an AbortError from its own timeout has not been cancelled by a shutdown and is still reported as a failure. isCancellation() is exported for handlers that catch our abort and rethrow, explicitly not as the decision.
  • RunSummary gains cancelled and inFlight, so an abandoned attempt is not reported as a fault.
  • reset() installs a fresh controller, so a reused framework is not permanently cancelled.
  • maintenance.ts checks the signal before pruning — the memoised Horizon clients are left alone if a shutdown lands between the job being picked up and the prune running — and gains shutdownMaintenance() plus cancelMaintenance() for an unload handler that cannot await.

This composes with the existing lifecycle and idempotency behaviour rather than replacing it: the transitions used (running --fail--> retrying, running --exhaust--> dead_lettered) already exist in the worker_job table, so nothing outside that table can be reached, and the enqueue idempotency records are untouched.

Verification

No test runner in this environment, so core/workers/__tests__/shutdown.test.ts was not executed. Instead the framework was run directly with node's TypeScript type stripping against the real modules — 55 assertions across 12 scenarios — and the committed test file's own expectations were replayed separately and all matched.

That caught three defects in the first draft, which is the main argument for having run it:

  • drain() waited for inherited work but did not count it, so a caller that drained and then awaited saw the job complete and a summary that never mentioned it
  • shutdown classified from the post-drain state, so a never-settling handler looked merely pending rather than interrupted — the opposite of the honest answer
  • cancellation was inferred from the error's class, which misreported a handler's own timeout as a shutdown

Every interleaving in the committed tests is deterministic: shutdown is awaited and handlers coordinate through a promise the test controls, so no assertion depends on timing.

npm run test is the outstanding check for a machine with dependencies.

Closes #142

…Protocol#142)

A maintenance job could be reported as completed after its side effect was
abandoned, and there was no way to ask what work was in flight when a process
stopped. Two things were wrong.

`runJob` invoked `registration.handler(...)` and discarded the result, so a
promise-returning handler was moved to `succeeded` before it had done anything
and its rejection escaped as an unhandled rejection. The framework now awaits
the handler: a sync handler settles inline, an async one is tracked so callers
can wait for a final outcome, and `drainDueJobs()` reports the rest as
`inFlight` rather than pretending they finished.

Nothing could be cancelled, because a handler had no way to learn that it
should stop. `WorkerContext` now carries an `AbortSignal` covering the whole
framework, and the contract per state at shutdown is explicit:

- queued / retrying — never started, left exactly as they are and reported as
  `pending`. Nothing was abandoned, so the next process re-runs them.
- running — the attempt was abandoned. Moved to `retrying`, or `dead_lettered`
  once the attempt budget is spent, and reported as `interrupted`. It is never
  marked `succeeded`.
- settled — untouched, counted in `settled`.

Once cancelling, neither `drainDueJobs` nor `processJob` starts new work, so a
handler is not re-entered after a shutdown. `shutdown()` aborts, waits for
in-flight handlers up to a timeout, and classifies from the state captured *at
abort time* — a handler that ignores its signal and never settles is still
`running` with an unknown side effect, which is exactly the job a caller most
needs told about, so it is reported as interrupted rather than pending.

Cancellation is decided by the framework's own signal and nothing else. A
handler that throws an `AbortError` from its own timeout has not been cancelled
by a shutdown and is still reported as a failure; `isCancellation()` is exported
for handlers that catch our abort and rethrow, not as the decision itself.

`RunSummary` gains `cancelled` and `inFlight` so an abandoned attempt is not
reported as a fault, and `reset()` installs a fresh controller so a reused
framework is not permanently cancelled.

`maintenance.ts` checks the signal before pruning — the memoised Horizon clients
are left alone if a shutdown lands between the job being picked up and the prune
running — and gains `shutdownMaintenance()` for an orderly teardown plus
`cancelMaintenance()` for an unload handler that cannot await.

Tests in core/workers/__tests__/shutdown.test.ts cover shutdown at each state
and cover the async-handler bug directly. Every interleaving is deterministic:
shutdown is awaited and handlers coordinate through a promise the test
controls, so no assertion depends on timing.

Verification: no test runner in this environment, so the suite was not executed.
Instead the framework was run directly with node's TypeScript type stripping
against the real modules (55 assertions across 12 scenarios, and the committed
file's own expectations replayed separately), which caught three defects in the
first draft: `drain()` waited for inherited work without counting it, shutdown
classified from the post-drain state so a never-settling handler looked merely
pending, and cancellation was being inferred from the error's class rather than
the signal.
@drips-wave

drips-wave Bot commented Sep 27, 2026

Copy link
Copy Markdown

@luciusverus-cyber Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@BigDella
BigDella merged commit bdc678f into Parcel-Protocol:main Sep 27, 2026
1 check failed
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.

[Workers] Add cancellation and shutdown-drain semantics

2 participants