Repository navigation
Conversation
When a process restarts within the ZooKeeper session timeout, the ephemeral node of its previous session still exists at the announcement path, so the announcer's create fails with NODEEXISTS. Both announcers relied on their cache to report the removal of that node, but PathChildrenCache and CuratorCache only report removals of nodes they have already loaded. If ZooKeeper deletes the stale node before the cache's initial load sees it, the announcement is never re-created and the process stays unregistered until it restarts. Handle NODEEXISTS in a create callback: set an exists watch on the path and re-create the announcement once the node is gone, as Curator's PersistentNode does. All re-create paths, including the existing cache listeners and the reconnect handler, now go through reinstateAnnouncement, which uses the current payload and skips paths that were unannounced in the meantime. Creates and exists checks that fail for any other reason are logged as warnings instead of being dropped. The existing tests waited for CuratorListener CREATE events, which Curator does not deliver once a create has a callback, so they now poll until the node exists and belongs to the announcer's session, using helpers in CuratorTestBase. New tests reproduce the race deterministically, both when the stale node is removed before the exists watch is set and after it.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
NodeAnnouncer had no reconnect handling, so if its own session expired while it waited for a stale node to go away, the exists watch was lost and nothing re-created the announcement unless the cache happened to hold the stale node. Re-announce every path on RECONNECTED: the create hits NODEEXISTS again and watches the node from the new session. Both announcers read the payload and then queued the create, so an unannounce running in between could leave a node behind. Queue the create inside computeIfPresent on the announcement's entry: a concurrent unannounce now either removes the entry first or waits, and its delete follows the create. createAnnouncement now wraps Curator's checked exception itself, which removes two try/catch blocks.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
Follow-up assessment
The author's latest replies address all three findings from the previous review. NodeAnnouncer now re-announces paths on RECONNECTED to re-arm stale-node watches after session loss, and both announcers queue reinstatement inside computeIfPresent, so a concurrent unannounce or stop cannot leave a replacement node behind. I reviewed the current head for remaining watcher, connection-lifecycle, and shutdown races and found no additional actionable issues.
Reviewed 5 of 5 changed files.
Validation: git diff --check passed for all five changed files. No tests or builds were run; this was a static follow-up review.
This is an automated review by Codex GPT-5.6-Luna(max)
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟢 Approval recommended
No actionable issues found in the current head. The head changed only by merging master; the generated incremental diff contains upstream changes but no overlap with this PR's five files. I rechecked all five current PR files, including the stale-node watch, reconnect and unannounce ordering, and deterministic race tests.
Reviewed 5 of 5 changed files. Static review only; no tests or builds were run.
This is an automated review by Codex GPT-5.6-Luna(max)
2 participants
Description
When a process is killed (for example by SIGKILL after a container OOM) and restarts on the same host and port before its old ZooKeeper session times out, the old session's announcement node is still there. The new process's create fails with
NODEEXISTS, which was silently ignored, so recovery depended on the announcer's cache noticing when the old node goes away.That only works if the cache had already loaded the old node:
PathChildrenCacheandCuratorCacheonly report removals of nodes they know about. If the old session expires first, nothing re-creates the announcement, and the process stays undiscoverable until its next restart, even though it can still become leader. We hit this in production: the stale node disappeared about 30ms after a restarted Coordinator announced itself, and when that Coordinator later became leader, other services failed withissued redirect to unknown URL. BothPathChildrenAnnouncer(the default) andNodeAnnouncerare affected.Re-announce once the old node is gone
On
NODEEXISTS, both announcers now watch the path, as Curator'sPersistentNodedoes, and re-create the announcement as soon as the old node is deleted, or right away if it already is. Other create or watch failures are logged instead of ignored. All re-create paths, including the existing cache listeners and the reconnect handlers, now go through one method that uses the current payload and queues the create while holding the announcement's entry, so a concurrent unannounce either prevents the create or deletes the node afterwards.NodeAnnouncernow also re-announces after reconnecting, asPathChildrenAnnounceralready did, so a watch lost with an expired session is set again.Alternatives: deleting the old node when it belongs to another session could make two live processes with the same host and port delete each other's announcements. Switching to
PersistentNodewould mean rewriting both announcers.Tests
New tests in both announcer test classes reproduce the race deterministically, with the old node disappearing both before and after the watch is set.
NodeAnnouncerTestalso covers the announcer's own session expiring while it waits for the old node to go. They fail without the fix and passed 25 of 25 repeated runs with it. The existing tests waited for CuratorCREATEevents, which are not delivered once a create has a callback, so they now poll until the node belongs to the announcer's session. As a result,testSessionKilledtakes about one session timeout (~15s).Related: #20339 is triggered by the same remove-and-re-add of a discovery node. This PR neither fixes nor causes it.
Release note
Fixed a race in ZooKeeper-based service discovery where a service that restarted within the ZooKeeper session timeout could stay unregistered until its next restart.
This PR has: