Repository navigation
pool: stop local work when cleanup-state reads fail - #90
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A failed preliminary cleanup-state read must not leave a node accepting work without attempting to stop its local handlers. Previously,
Node.Closereturned 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
Closemethod. The original preliminary error is returned even if subsequent ordinary closure succeeds, and remains discoverable througherrors.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: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 --checkpassed.go.modandgo.sumare 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.