HarperFast / HarperFast/harper

Follow-up: residual partial-failure risks in table-drop hardening (#1246)

Open
#1,276 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
89
Forks
10
Avg merge
2d 6h
Merged PRs (30d)
200

Description

Follow-up to #1246 (atomic table drops / interrupted-drop completion). Both items below are residual partial-failure windows that #1246 intentionally left in place — they're guarded in practice by the rocksdb-js column-family eviction fix (HarperFast/rocksdb-js#647, shipped in `^2.1.0`), but are worth closing for true defense-in-depth.

### 1. Create-path `catch` is lock-only, not state-rollback

In `resources/databases.ts`, `table()`'s create branch wraps `setTable(...)`, the `NEXT_TABLE_ID` increment, and the final `attributesDbi.put(dbiName, primaryKeyAttribute)` in a `try`. The `catch` correctly releases the exclusive `update-attributes` spin lock and rethrows (the wedge fix), but it does **not** roll back partial in-memory state. If the throw lands after `setTable` (e.g. the final `put` fails), the `tables` map holds a half-built `Table` and `NEXT_TABLE_ID` is already bumped.

Likely benign (table id is monotonic; a subsequent create overwrites via `setTable`, and `getSync(dbiName)` is undefined so it re-enters cleanly), but the lingering half-built `Table` is a latent footgun. Consider resetting in the `catch`, or documenting why the leftover is safe.

### 2. `completeInterruptedDrop` removes catalog rows even when a CF drop keeps failing

`dropTable()` and `completeInterruptedDrop()` take **opposite** stances on a failing column-family drop:

- `dropTable()` awaits the drops and *keeps* the catalog (tombstoned) on failure, so it can be retried.
- `completeInterruptedDrop()` tolerates per-store drop failures (catch + warn) and then removes the catalog rows **unconditionally** at the end.

So a CF that *persistently* fails to drop becomes an orphan with no catalog row left to reconcile against — it's then invisible and unreconcilable on future boots. The binding fix makes this safe today, but it's the inverse of the failure mode #1246 set out to fix. At minimum this asymmetry should be documented in the DESIGN.md drop section (which currently lists the index-vs-primary partial-failure window but not this one); better, only remove a catalog row once its store drop has actually succeeded.

### Related
- Builds on the node-local `dropping` tombstone introduced in #1246; a natural pairing with the replicated-DROP-event work in #1212.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.