fix(writer-lease): allow takeover of an expired lease on stores witho…

[SanthoshRaaj-KR]

Summary

On LocalFileSystem (CLOUD_PROVIDER=local), a node that comes back from an unclean restart can no longer write. Reads continue returning 200 and /readyz stays green, so the node appears healthy while mutations fail with an opaque 500.

Reported in #196.

Root cause

acquire_or_renew_inner renews a lease with PutMode::Update(version) once a lease file already exists. LocalFileSystem does not support conditional updates, so this returns NotImplemented. There is a fallback to a plain put for exactly this case, but it was gated on same_holder.

holder_id is a fresh ULID per process (process_holder_id()). After a restart, the new process has a different holder ID from the one stored in the existing lease. The fallback is therefore skipped, and acquire_or_renew returns NotImplemented on subsequent write attempts. Reads never reach this path because ensure_local_writer is write-path only, which is why the failure is invisible to health checks.

The same_holder gate was redundant for safety. The ownership check immediately above the write already returns NotCellWriter when a lease is still active under a different process. Therefore, anything that reaches the write is an absent, released, expired, or already-owned lease. None of these cases requires compare-and-swap for this overwrite.

release_stored already performs this fallback without the same_holder check, so this change also makes the renew and release paths consistent.

Why this wasn't caught

A graceful shutdown calls release_stored, which deletes the lease file on LocalFileSystem. The next start therefore takes the PutMode::Create path and succeeds.

Only an unclean stop leaves the lease file behind. scripts/runtime_smoke.sh also clears its store on every run, so it does not exercise a restart with existing state.

Changes

• Remove the same_holder condition from the NotImplemented fallback in acquire_or_renew_inner and document the invariant that makes the plain put safe.
• Add expired_lease_is_recoverable_on_a_store_without_conditional_update.

Testing

The new test is a LocalFileSystem analogue of the existing fresh_observer_does_not_restart_an_expired_lease_window, which runs on InMemory. InMemory implements conditional update and therefore does not reach the branch covered by this fix.

The test verifies both sides of the invariant: a restarted process is still refused while the incumbent lease is active, and succeeds once that lease has expired.

Without the fix, the test fails with:

ObjectStore(NotImplemented {
  operation: "put_opts with mode PutMode::Update",
  implementer: "LocalFileSystem(file:///tmp/.tmpW6x16O)"
})

With the fix, cargo test --locked --lib passes 204/204.

restarted_process_with_same_node_id_cannot_share_the_lease still passes, so a restarted process cannot take over a lease that is genuinely still held.

Only the NotImplemented branch is changed, so behavior is unchanged for stores that implement conditional updates.

Not verified locally

I was unable to run just fence or just stress locally. Both exercise writer takeover and require the full native feature set, which my environment cannot currently build. These should be verified by CI or a reviewer.

Related

• Manifest GC fails permanently under the documented CLOUD_PROVIDER=local config: put_opts with PutMode::Update unimplemented by LocalFileSystem #81 reports the same put_opts / PutMode::Update / NotImplemented failure on LocalFileSystem and may also be resolved by this change.
• The opaque "internal query execution error" that hides this class of failure is tracked separately in Query errors are replaced with "internal query execution error" on both transports, so clients cannot diagnose failures #107 / HTTP and Bolt query adapters suppress ConditionalWriteConflict and ControlMetadataConflict as internal query errors instead of conflicts #204 / HTTP and Bolt query adapters suppress RetryExhausted, SnapshotExpired, and UnknownShard as internal 500 errors #206 / HTTP and Bolt query adapters suppress WriteRequiresWriter, ReadOnlyShardStorage, and CellDropped as internal 500 errors #208.
• The 400-level responses in Self-hosted graph-node /v1/graphs/{graph_id}/query has no working mutation/delete cycle in Docker local mode (?) #196, including standalone CREATE, CREATE ... RETURN, MERGE, and UNWIND ... CREATE, are separate query-engine limitations and are not addressed by this PR.

[Copilot]

Copilot review overview

🟡 Changes recommended

The fallback overwrite is not atomic with the ownership check and could permit concurrent writers.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Prevent non-atomic lease takeover from overwriting renewed leases · New

What changed in this PR

Fixes writer-lease recovery after unclean LocalFileSystem restarts and adds regression coverage.

Changes:

• Removes the holder-ID gate from the fallback overwrite.
• Adds an expired-lease takeover test.

File	Summary
src/​engine/​writer_lease.rs	Updates lease fallback logic and adds coverage.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[greptile-apps]

Confidence Score: 4/5

The PR is not safe to merge until concurrent expired-lease takeover on LocalFileSystem preserves the single-writer guarantee.

Summary

This PR allows a process to recover an expired writer lease on object stores that do not support conditional updates and adds LocalFileSystem regression coverage.

• Removes the holder-identity restriction from the NotImplemented fallback.
• Falls back to an unconditional lease-file overwrite.
• Tests refusal during the incumbent lease window and successful takeover after expiration.
• The unconditional takeover introduces a race in which concurrent contenders can both report successful acquisition.

Diagram

sequenceDiagram
    participant A as Replacement A
    participant L as LocalFileSystem lease
    participant B as Replacement B
    A->>L: Read expired generation N
    B->>L: Read expired generation N
    A->>L: Conditional update to N+1
    L-->>A: NotImplemented
    B->>L: Conditional update to N+1
    L-->>B: NotImplemented
    A->>L: Unconditional overwrite N+1
    L-->>A: Success
    B->>L: Unconditional overwrite N+1
    L-->>B: Success
    Note over A,B: Both install valid local leases and return success

Loading

_{Reviews (1) · Last reviewed commit: "fix(writer-lease): allow takeover of an ..."}

[greptile-apps]

Comments Outside Diff

These findings sit on lines the diff does not cover, so they could not be posted inline. Each one leaves this list once its file changes.

•  Concurrent takeovers both succeed src/engine/writer_lease.rs:291 ▶

  When two replacement processes race for the same expired LocalFileSystem lease, both can read the expired record and pass the ownership check before either writes. This fallback then lets both unconditional overwrites succeed, after which each process installs a valid local lease and returns the same incremented generation. Because the write path trusts that local lease until renewal, both processes can operate as the writer until downstream SlateDB fencing detects one, breaking the lease's single-writer guarantee. This takeover needs an atomic local mechanism or equivalent serialization rather than a blind overwrite.

[openhack-agent]

OpenHack Summary

Security review of fix(writer-lease): allow takeover of an expired lease on stores witho…. 1 changed file; 1 finding at or above the low reporting threshold.

0   1   0   0

Confidence Score: 2/5

Review the findings below before merging.

Security merge-readiness rubric: 1 = critical, 2 = high, 3 = medium, 4 = low, 5 = no reportable findings. This score reflects scan findings, not a guarantee of correctness or complete coverage.

Files Needing Attention: src/engine/writer_lease.rs

Important Files Changed

• src/engine/writer_lease.rs (modified)

Prompt To Fix With AI

Review the findings for https://github.com/hydra-db/hydradb/pull/210 at commit 33fb235ea51a7f120f566fe7f31edf37c02f5ab7. Verify each finding against the current code before fixing it. Preserve unrelated changes and run focused regression tests.

### Issue 1: [P2] Non-atomic lease takeover on local object stores enables split-brain writers
Vulnerability type: Business logic flaw
src/engine/writer_lease.rs:280

The pull request changes the no-conditional-update path from `Err(ObjectStoreError::NotImplemented { .. }) if same_holder` to an unconditional fallback. On LocalFileSystem, multiple processes can observe an expired lease, compute the same next generation, and pass the non-atomic ownership check before any of them writes. Each then overwrites the lease file and reports success, so more than one process can believe it owns the same cell lease. The deleted behavior failed closed for stale takeovers, preventing this race but also stranding leases after an unclean restart. The regression is therefore the removal of atomic takeover protection, not merely lease recovery.

Recommendation: Do not use an unconditional overwrite as the authorization mechanism for stale lease takeover. Preserve fail-closed behavior for non-same-holder takeovers unless the store provides an atomic compare-and-swap or create-only coordination primitive. For LocalFileSystem, implement an atomic takeover protocol that serializes the read-check-write window (for example, an exclusive lock/create-only coordination object), and include a per-attempt nonce with post-write verification so a contender that loses the race relinquishes the lease and fails or retries. Add a concurrent expired-lease test asserting that at most one contender succeeds.

Last reviewed commit: 33fb235 · View review on OpenHack

---

TIP: Mention @openhack-agent in a PR comment to request a review or ask a question. Use @openhack-agent fix all for every finding, or @openhack-agent fix unresolved threads for open review threads only.

[openhack-agent]

OpenHack reviewed this commit. See the OpenHack Summary for the confidence score and fix actions.

[openhack-agent]

Non-atomic lease takeover on local object stores enables split-brain writers

Vulnerability type: Business logic flaw

The pull request changes the no-conditional-update path from Err(ObjectStoreError::NotImplemented { .. }) if same_holder to an unconditional fallback. On LocalFileSystem, multiple processes can observe an expired lease, compute the same next generation, and pass the non-atomic ownership check before any of them writes. Each then overwrites the lease file and reports success, so more than one process can believe it owns the same cell lease. The deleted behavior failed closed for stale takeovers, preventing this race but also stranding leases after an unclean restart. The regression is therefore the removal of atomic takeover protection, not merely lease recovery.

Location: src/engine/writer_lease.rs:280

Recommendation:

Do not use an unconditional overwrite as the authorization mechanism for stale lease takeover. Preserve fail-closed behavior for non-same-holder takeovers unless the store provides an atomic compare-and-swap or create-only coordination primitive. For LocalFileSystem, implement an atomic takeover protocol that serializes the read-check-write window (for example, an exclusive lock/create-only coordination object), and include a per-attempt nonce with post-write verification so a contender that loses the race relinquishes the lease and fails or retries. Add a concurrent expired-lease test asserting that at most one contender succeeds.

Prompt To Fix With AI

Review the findings for https://github.com/hydra-db/hydradb/pull/210 at commit 33fb235ea51a7f120f566fe7f31edf37c02f5ab7. Verify each finding against the current code before fixing it. Preserve unrelated changes and run focused regression tests.

### Issue 1: [P2] Non-atomic lease takeover on local object stores enables split-brain writers
Vulnerability type: Business logic flaw
src/engine/writer_lease.rs:280

The pull request changes the no-conditional-update path from `Err(ObjectStoreError::NotImplemented { .. }) if same_holder` to an unconditional fallback. On LocalFileSystem, multiple processes can observe an expired lease, compute the same next generation, and pass the non-atomic ownership check before any of them writes. Each then overwrites the lease file and reports success, so more than one process can believe it owns the same cell lease. The deleted behavior failed closed for stale takeovers, preventing this race but also stranding leases after an unclean restart. The regression is therefore the removal of atomic takeover protection, not merely lease recovery.

Recommendation: Do not use an unconditional overwrite as the authorization mechanism for stale lease takeover. Preserve fail-closed behavior for non-same-holder takeovers unless the store provides an atomic compare-and-swap or create-only coordination primitive. For LocalFileSystem, implement an atomic takeover protocol that serializes the read-check-write window (for example, an exclusive lock/create-only coordination object), and include a per-attempt nonce with post-write verification so a contender that loses the race relinquishes the lease and fails or retries. Add a concurrent expired-lease test asserting that at most one contender succeeds.

---

TIP: Reply @openhack-agent or @openhack-agent fix this to fix this finding. To ask a question, mention @openhack-agent followed by your question.

[SanthoshRaaj-KR]

I added the requested race test and confirmed the reviewer's concern.

The test starts two replacement processes and has both attempt to take over the same expired lease on LocalFileSystem. It fails immediately on this branch:

assertion `left != right` failed: exactly one replacement may win the
takeover, got a=Ok(2) b=Ok(2)

Both processes acquire the lease and both return generation 2. This confirms that the current patch breaks the single-writer guarantee, so it should not merge in its current form.

I don't think there is a small safe fix for this:

• delete followed by PutMode::Create is not safe because the unconditional delete could remove a lease that another process has already created.
• Checking node_id does not solve the problem because both processes in the restart scenario can have the same GRAPH_NODE_ID.
• Read-back verification reduces the race window but cannot eliminate it.

With only get / put / put-if-absent / delete available, safely replacing an expired lease appears to require either a conditional delete or real CAS. LocalFileSystem provides neither, which likely explains why the existing implementation fails closed.

One possible design is to use generation-suffixed claim files: create lease.gen{N+1} so that only one contender can successfully create a given generation, with readers selecting the highest valid generation. However, this would change the on-disk lease format, although the path is already versioned as v2, and would also require handling stale claim files.

I don't think I should make that design decision in this PR, so I've left the PR as a draft for now. I'd appreciate guidance on whether this is the direction you'd like to take.