Skip to content

pool: stop local work when cleanup-state reads fail - #90

Merged
raphael merged 2 commits into
mainfrom
close-local-work-after-cleanup-read-error
Oct 2, 2026
Merged

raphael merged 2 commits into
mainfrom
close-local-work-after-cleanup-read-error

Conversation

@raphael

@raphael raphael commented Oct 2, 2026

Copy link
Copy Markdown
Member

A failed preliminary cleanup-state read must not leave a node accepting work without attempting to stop its local handlers. Previously, Node.Close returned immediately when its first Redis HMGet failed, before canceling scheduled planning, joining worker intake, or calling Stop for accepted jobs. A caller could receive that error while local work continued using dependencies supplied by the application. This is a verified source path; no production occurrence or frequency is asserted.

Close now passes that error branch through the existing ordinary node-close operation and returns errors.Join(readError, closeError). For a node that is not already closed, that operation marks it closing, cancels and joins scheduled work, attempts every local worker's shutdown, joins node work and terminal dispatch outcomes, and proceeds with ordinary cleanup only when its existing prerequisites succeed. Admission closes at the existing local state transition, not at method entry or while the preliminary read is pending.

Worker remains the sole owner of handler acceptance and release. Its existing Stop path joins intake, serializes accepted-handler work, retains failed Stops, and removes successful local jobs so later cleanup retries do not repeat successful Stops. Node owns saved-job recovery and resource detachment. Existing generation checks still decide which Redis resources can be changed; an unreadable cleanup marker does not establish completed pool cleanup or shutdown.

Errors and completion

Callers still send a context to the same Close method. The original preliminary error is returned even if subsequent ordinary closure succeeds, and remains discoverable through errors.Is. Stop, worker-stream, pending-dispatch, recovery, and detach errors retain their existing propagation and retry behavior. Failed obligations remain available for another Close with a fresh context.

This error path newly publishes local closure only after ordinary close succeeds. If another close operation has already closed the node, its existing state is preserved. The failed read itself establishes neither pool-wide cleanup completion nor shutdown. Error presence alone does not prove whether application work remains live. A failed Stop is not proof that a handler released its work, and this change cannot force a nonreturning handler or legacy planner to terminate. Close does not close the caller's Redis client or stop remote workers.

Readable-marker, cached-completion, completed-generation, Shutdown, and RemoveWorker paths are unchanged. No worker implementation, requeue/generation script, exported API, private production state, dependency, or numeric limit changes. The code change is one error branch plus its GoDoc; the README explains the stronger error-path behavior.

Compatibility and verification

There is no API or wire migration, storage-format change, or SDK regeneration. Existing callers keep the same signatures. Independently deployed binaries acquire this behavior when rebuilt; there is no new cross-node protocol or deployment order. Close can take longer on the formerly immediate error path because it now waits for the existing cooperative shutdown operation. Rolling back restores the old early-return behavior without data conversion.

With Go 1.26.3 on darwin/arm64, the focused synthetic suite passed all 20 top-level tests and 44 subtests with -race, -count=1, and a 45-second test timeout. The final command was:

GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off GOWORK=off GOTELEMETRY=off go test -mod=readonly -race ./pool -run '^TestLocalShutdown' -count=1 -timeout=45s -v

Four new public-Close tests exercise the actual HMGet selection, an established stream-generation handle, cancellable planner invocation and owned join ordering, actual handler acceptance, worker and node reader closure, both successful and failing workers, preserved error chains, canceled contexts, retained dispatch obligations, and cleanup retry without repeating successful Stops. The same final tests fail against the original Close branch in the error cases; readable-marker cases continue to pass. Existing tests preserve pre-handler ErrRequeue classification, genuine Start failures, saved-record validation, and ordinary/completed-cleanup retry behavior.

All commands were answered in memory by go-redis hooks; the fixtures reject dialing and assert zero dial and pipeline attempts. Planning is registered in the existing node-owned wait group and invokes PlanContext; these tests do not run distributed scheduler scripts. The completed-ordinary-closure state test uses the existing cleanup callback seam before invoking public Close; it does not prove Redis detachment. No Lua, distributed barrier, Redis/network fault experiment, or production probe was run. The draft remains pending its normal Linux CI checks.

git diff --check passed. go.mod and go.sum are byte-identical to the base, and the 21 module identities used by the pool package match. A complete module listing was unavailable with offline lookups disabled; no complete module-graph comparison is claimed.

After the test run, only GoDoc and README wording was corrected. Executable Go code and all test bytes are unchanged; tests and lint were not rerun for that documentation correction.

Review Node.Close, closeWithCleanup, Worker.stopLocal, and the new public-path tests first. The stronger local attempt on a preliminary read failure is the scope; application-owned dependency closure after an unsuccessful Stop remains a separate responsibility.

@raphael
raphael marked this pull request as ready for review October 2, 2026 20:54
@raphael
raphael merged commit b34ad25 into main Oct 2, 2026
6 checks passed
@raphael
raphael deleted the close-local-work-after-cleanup-read-error branch October 2, 2026 20:55
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