[core][gcs] Publish node death before persist; drop RocksDB soft-durability (REP-64)

[jhasm]

Why are these changes needed?

Follow-up to #63657 (embedded RocksDB GCS backend, REP-64) that fixes the underlying pub/sub issue at its root and removes the temporary "soft-durability" workaround.

Root cause (proven in #64187)

GCS published the node-death notification from inside the node table write's completion callback — publish-after-persist. Owners of objects/tasks react to a node's death only from that single pushed pub/sub notification (core_worker.cc on_node_change, DEAD branch), and once subscribed there is no periodic owner-side re-poll. Gating the publish on the durable write therefore couples cluster-wide death detection to persist latency: under a slow backend (the RocksDB GCS with per-write fsync) the notification is delayed enough to strand object recovery and hang test_dynamic_generator_reconstruction_nondeterministic[None-*] in list(gen).

The provenance PR #64187 demonstrates this deterministically on the in-memory GCS (timing injected), confirming the bug is in Ray core and backend-agnostic.

Fix (F3 — publish node death before persist)

Publish node death on the in-memory DEAD transition, posted onto the GCS io_context and decoupled from the durable write. Applied to both node-death paths:

• GcsNodeManager::InternalOnNodeFailure — health-check failure.
• GcsNodeManager::HandleUnregisterNode — graceful unregistration.

Properties:

• Relaxes broadcast-after-durable ordering only for the node channel. Safe because node death is a terminal, restart-re-derivable transition (re-established by health checks on GCS restart).
• Not generalized to the actor channel. This is not because the actor channel has extra client-side self-healing that nodes lack — in fact the two are symmetric: both fetch current state on subscribe and re-fetch on GCS restart (AsyncResubscribe), and neither periodically re-polls an already-established subscription. The difference is durability semantics: actor state is not terminal and can legitimately change (e.g. RESTARTING → ALIVE) and must survive a GCS crash in durable order, so its publish must stay gated on persist. Node liveness is terminal and re-derivable, so relaxing the ordering is safe.
• Keeps strict durability; smallest change that removes the proven cause at its source.

We deliberately did not add the optional owner-side fallback re-poll (F5 in #64187): the demonstrated failure is a slow publish, not a dropped one, and F5 would add a permanent periodic GCS poll to every owner for a hypothetical the pub/sub layer already makes unlikely. It remains an isolated future option if a genuinely dropped notification is ever observed.

Remove the soft-durability workaround

With the root cause fixed, all GCS tables use strict per-write WAL fsync (sync = true) again. Deletes SoftDurableTables() and simplifies RocksDbStoreClient::SyncWriteOptions() (now returns a const & to a shared constant, no per-write copy). The NODE/ACTOR relaxation is no longer needed.

How do we know this is safe?

The ordering guarantee (death is broadcast before, and independently of, the durable write) is covered end-to-end and at the unit level:

• End-to-end regression — test_dynamic_generator_reconstruction_nondeterministic[None-*] (python/ray/tests/test_generators.py) is the exact scenario the bug hangs. It passes on the RocksDB GCS with strict fsync and no soft-durability, i.e. purely because of F3.
• Deterministic unit ordering tests (gcs_node_manager_test.cc, new):
  • TestNodeFailurePublishesDeathBeforePersist — health-check path: after OnNodeFailure, running exactly one queued handler broadcasts DEAD while the persist-completion callback has not yet run.
  • TestUnregisterNodePublishesDeathBeforePersist — graceful-unregister path: the DEAD publish fires before the persist-completion callback (which is what writes the DEAD export event).
    Both fail against the old publish-after-persist behavior and pass with F3, so they lock in the fix.
• Provenance proof ([DO NOT MERGE][core][gcs] Provenance: node-death lost-wakeup hangs generator reconstruction (REP-64) #64187) demonstrates the same lost-wakeup on the in-memory GCS with injected timing, independent of any storage backend.

Practically-possible scenarios and why they remain safe: (a) slow persist — publish is already decoupled; (b) GCS restart before persist — node death is re-derived by health checks and clients re-fetch on resubscribe; (c) dropped push (not observed) — out of scope for this PR and independently addressable via the F5 backstop without touching F3.

Related issue number

• Root-cause analysis / proof: [DO NOT MERGE][core][gcs] Provenance: node-death lost-wakeup hangs generator reconstruction (REP-64) #64187
• Introduced the workaround this PR removes: [core][gcs] Add embedded RocksDB storage backend for GCS fault tolerance (REP-64) #63657

Checks

• gcs_node_manager_test (incl. new ordering tests), node_manager_export_event_test pass
• rocksdb_store_client_test, rocksdb_smoke_test pass
• gcs_server_lib + rocksdb_store_client build cleanly
• clang-format / black / ruff / pydoclint clean

[gemini-code-assist]

Code Review

This pull request decouples GCS node-death notifications from the durable storage write, publishing them immediately upon the in-memory state transition to DEAD. This resolves a reconstruction race hang under slow durable backends like RocksDB. Consequently, the "soft-durability" workaround (skipping fsync) for certain tables in the RocksDB store client has been removed, restoring full synchronous fsync for all writes. The review feedback suggests optimizing SyncWriteOptions in RocksDbStoreClient by returning a const reference to a static rocksdb::WriteOptions instance to avoid unnecessary copy overhead on every write operation.

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{Reviewed by Cursor Bugbot for commit 5d89fcb. Configure here.}

[rueian]

#64187 is probably not a correct reference. We never merged it.

[jhasm]

Good catch — fixed in 0081cde. You're right that #64187 was never merged; the fix actually lives in this PR. #64187 is the provenance/proof PR that demonstrates the lost-wakeup deterministically on the in-memory GCS (with injected timing), so I kept it only as an explicitly-labeled "root-cause proof, not merged" pointer and removed the misleading "fixed in #64187" wording from the test comment (and the secondary cross-reference in gcs_node_manager.cc). Happy to drop the #64187 mention entirely if you'd prefer no reference to an unmerged PR in the code.

[rueian]

Use io_context_.post.

[jhasm]

Done in 0081cde — switched to io_context_.post(handler, "GcsNodeManager.PublishNodeDeathOnUnregister"). Using the instrumented member (instead of the boost::asio::post free function) restores the handler count/queueing/execution stats and a debug name, and matches the convention used elsewhere in gcs (e.g. gcs_placement_group_manager.cc, postable.h). Also dropped the now-unused <boost/asio/post.hpp> include.

[rueian]

Use io_context_.post.

[jhasm]

Done in 0081cde — same change here, now io_context_.post(handler, "GcsNodeManager.PublishNodeDeathOnFailure").

[rueian]

LGTM. Let's wait for tests to pass.