{181186021} Sc timepart resume fix

[WalidNejmi]

This is not an easy fix--multiple bugs were found so might drop this PR, but running rr tests first and adding a summary comment for future ref:

Make time partition schema change resume safe

Summary

A time-partition schema change is a multi-table schema change: each shard runs its own schema change, but all shards must commit or abort as one logical operation.

sc_timepart was intermittently failing after killing the master during an ALTER because one shard could be left marked as "in schema change" without the seed required to resume it. Making the test reliably exercise failover then exposed use-after-free bugs in the time-partition resume path.

This change hardens the schema-change lifecycle around failover:

• retry schema-change seed writes and deletes when an internally-owned llmeta transaction loses a deadlock;
• fail schema-change startup if its seed cannot be persisted;
• stop using a schema_change_type after start_schema_change() may have freed it;
• represent failed shard resumes with safe placeholder objects so the whole partition schema change can abort cleanly;
• clear stale in-schema-change markers for shards that never successfully restarted;
• make the existing sc_timepart test reliably exercise resume;
• add deterministic fault-injection tests for seed failures and time-partition resume failures;
• fix several pre-existing memory errors exposed by ASAN while exercising these paths.

---

Background

When a schema change starts, Comdb2 persists a seed in llmeta.

That seed is part of the durable state required to reconstruct the schema change after a master failover.

For a time partition, an ALTER may involve several physical shard tables:

flowchart TD
    A["ALTER time partition"] --> B["Shard t0 schema change"]
    A --> C["Shard t1 schema change"]
    A --> D["Shard t2 schema change"]

    B --> E["Persist SC seed"]
    C --> F["Persist SC seed"]
    D --> G["Persist SC seed"]

    E --> H["Mark shard in schema change"]
    F --> I["Mark shard in schema change"]
    G --> J["Mark shard in schema change"]

Loading

If the master dies while the operation is running, the new master discovers the shards that were in schema change, retrieves their saved seeds, and resumes them.

The important invariant is:

A table must not be left recoverably marked as being in schema change unless the metadata required to resume that schema change was successfully persisted.

The failure that started this investigation violated that invariant.

---

Bug 1: schema-change seed writes gave up on the first deadlock

start_schema_change_tran() saves each table's seed through bdb_set_sc_seed().

When no transaction is supplied, bdb_set_sc_seed() opens its own llmeta transaction.

For a multi-table schema change, however, other shards may already be running asynchronously and writing llmeta at the same time.

The observed sequence was:

sequenceDiagram
    participant Main as SC startup
    participant T1 as Seed transaction for t1
    participant T2 as Async SC thread for t2
    participant LL as llmeta

    Main->>T1: Save t1 seed
    T1->>LL: Begin llmeta transaction

    T2->>LL: Update SC metadata for t2

    LL-->>T1: BDBERR_DEADLOCK
    T1-->>Main: Failure
    Main->>LL: Continue and mark t1 in schema change

    Note over Main,LL: t1 is now marked in SC<br/>without a durable seed

Loading

Previously, bdb_set_sc_seed() did not retry the deadlock.

Its abort could also overwrite the original bdberr, making the real cause less clear.

The caller then logged:

Couldn't save schema change seed

but continued starting the schema change.

If the master died afterward, the new master saw the table marked as in schema change but could not recover its seed:

Failed to determine host and seed!

At that point the shard could not be resumed and the multi-shard resume stopped.

Fix

bdb_set_sc_seed() now follows the normal llmeta deadlock-retry pattern when it owns the transaction:

flowchart TD
    A["Begin llmeta transaction"] --> B["Write schema-change seed"]

    B -->|Success| C["Commit"]
    B -->|Failure| D["Save original bdberr"]

    D --> E["Abort transaction"]
    E --> F["Restore original bdberr"]

    F --> G{"BDBERR_DEADLOCK?"}

    G -->|Yes| H["Optional deadlock backoff"]
    H --> I{"Retry limit reached?"}
    I -->|No| A
    I -->|Yes| J["Return failure"]

    G -->|No| J

Loading

The retry uses:

• gbl_maxretries
• gbl_llmeta_deadlock_poll

The original bdberr is preserved across bdb_tran_abort().

If the caller supplied a transaction, the helper does not independently abort and restart it; the error is returned to the transaction owner.

---

Bug 2: a schema change could still start without its seed

Retrying transient deadlocks fixes the common failure, but it does not guarantee that a seed write will always succeed.

For example:

• the retry limit could be exhausted;
• the llmeta write could fail for a non-deadlock reason.

Previously, even a permanent seed-save failure only produced a log message and schema-change startup continued.

That still allowed the invalid state:

LLMETA_IN_SCHEMA_CHANGE = present
LLMETA_SC_SEEDS         = missing

A future master cannot safely resume that schema change.

Fix

Saving the seed is now a prerequisite for starting a recoverable schema change.

If bdb_set_sc_seed() ultimately fails:

1. the schema-change running state is cleared;
2. iq->errstat is set to Failed to save schema change seed;
3. startup returns SC_LLMETA_ERR;
4. the schema change does not proceed without durable recovery metadata.

Conceptually:

flowchart TD
    A["Start schema change"] --> B["Persist SC seed"]
    B --> C{"Seed persisted?"}

    C -->|Yes| D["Continue schema-change startup"]
    C -->|No| E["Clear SC running state"]
    E --> F["Return SC_LLMETA_ERR"]
    F --> G["Do not publish an unrecoverable SC"]

Loading

This preserves the recovery invariant even when the failure is not transient.

---

Bug 3: schema-change seed deletes had the same deadlock weakness

The seed lifecycle is symmetrical:

flowchart LR
    A["Schema change starts"] --> B["Save seed"]
    B --> C["Schema change runs"]
    C --> D["Schema change completes or aborts"]
    D --> E["Delete seed"]

Loading

bdb_delete_sc_seed() used the same standalone llmeta transaction pattern as the old setter, but also did not retry deadlocks.

A transient deadlock during cleanup could therefore leave stale seed metadata behind.

Fix

bdb_delete_sc_seed() now uses the same rules as the setter:

• retry BDBERR_DEADLOCK when it owns the transaction;
• use gbl_maxretries;
• use gbl_llmeta_deadlock_poll;
• preserve the original bdberr across abort;
• do not independently retry a transaction supplied by the caller.

---

Bug 4: failed time-partition resume could use a freed schema-change object

Once sc_timepart was changed to reliably exercise failover, another product bug became reproducible.

The resume path created a new schema_change_type, linked it into the time-partition resume list, and then called:

start_schema_change(new_sc)

The problem is that start_schema_change() does not guarantee that the passed object is still alive after failure.

Several failure paths inside start_schema_change_tran() free the schema_change_type.

The old flow was effectively:

flowchart TD
    A["Allocate new_sc"] --> B["Link new_sc into resume list"]
    B --> C["start_schema_change(new_sc)"]

    C -->|Success| D["Continue"]
    C -->|Failure| E["start_schema_change may free new_sc"]

    E --> F["Caller writes new_sc->sc_rc"]
    F --> G["Finalizer later uses new_sc"]
    G --> H["Use-after-free / invalid mutex / double lifetime"]

Loading

This was observed as:

verify_sc_resumed_for_shard: failed to restart shard 't0'
[FATAL] pthread_mutex_lock(...) rc:22 Invalid argument

Fix

A failed start_schema_change() is now treated as consuming or invalidating the original pointer.

The caller never touches it again.

For a shard that has to be newly restarted:

flowchart TD
    A["Allocate new_sc"] --> B["start_schema_change(new_sc)"]

    B -->|Success| C["Link real SC into resume list"]

    B -->|Failure| D["Do not touch original new_sc again"]
    D --> E["Allocate failed-resume placeholder"]
    E --> F["Store table, UUID and failure rc"]
    F --> G["Link placeholder into resume list"]

Loading

The placeholder was never started. Its only purpose is to carry the failed shard's result into the common multi-DDL finalizer so the entire time-partition schema change can abort cleanly.

---

Bug 5: the existing-shard resume path had the same ownership problem

The same class of use-after-free existed in verify_sc_resumed_for_all_shards().

Previously it did roughly:

rc = start_schema_change(sc);

if (rc != SC_OK)
    sc->sc_rc = SC_ABORTED;

But sc may already have been freed by the failed call.

Fix

Before starting the schema change, the code saves the information needed afterward:

• next list element;
• table name;
• UUID.

If startup fails, the original list entry is replaced with new_failed_resume_sc().

The failed pointer is never dereferenced again.

The resume list therefore contains either:

successful resume
    -> real schema_change_type

failed resume
    -> fresh placeholder carrying the failure rc

rather than a mixture of valid and potentially freed pointers.

---

Bug 6: a shard that never restarted could keep its in-schema-change marker

A placeholder represents a shard whose schema change never actually restarted.

Because no schema-change worker was successfully started for that shard, the normal worker cleanup path does not run.

That could leave the shard's persistent LLMETA_IN_SCHEMA_CHANGE marker behind.

The next master could then discover that marker and try to resume the shard independently from the rest of the partition operation.

Fix

When the multi-DDL finalizer aborts, it now clears the in-schema-change marker for a shard that never started:

!sc->started

except when the failure was:

SC_CANT_SET_RUNNING

That case means another schema change owns the table, so its marker must not be cleared.

Conceptually:

flowchart TD
    A["Failed-resume placeholder"] --> B{"Did this SC ever start?"}

    B -->|Yes| C["Normal SC cleanup owns its state"]
    B -->|No| D{"SC_CANT_SET_RUNNING?"}

    D -->|No| E["Clear stale in-schema-change marker"]
    D -->|Yes| F["Leave marker for the SC that owns the table"]

Loading

---

Test issue 1: sc_timepart did not reliably exercise resume

The original test attempted to guarantee that the schema change was still running when the master was killed by setting:

PUT SCHEMACHANGE CONVERTSLEEP 10

However, CONVERTSLEEP is node-local.

The test set it on the master and through one default-routed connection, while the ALTER itself could be parsed on a different node.

When the ALTER landed on a node without CONVERTSLEEP, it could finish in about one second.

The test then killed the master three seconds later, after the schema change was already complete.

That meant the "resume" test sometimes passed without performing any resume at all.

Fix

CONVERTSLEEP is now set explicitly on every cluster node before the ALTER.

That guarantees that whichever node parses the ALTER uses the intended delay and the master kill occurs while the schema change is still active.

---

Test issue 2: the ALTER could race with deletion of the retired shard

During a time-partition rollout, the view can switch to:

t2;t1

while the retired t0 still physically exists until delete_lag expires.

During that window:

logical view:
    t2
    t1

physical tables:
    t2
    t1
    t0  <- retired, waiting to be dropped

Because shard names cycle, t0 is also a possible future shard name.

_next_shard_exists() can therefore interpret the retiring t0 as the next shard.

An ALTER started during this window can include t0 while the rollout thread is concurrently dropping it.

This is a separate product/design issue; this change does not redefine the shard-generation model.

Fix to the normal regression test

Before starting the regular resume scenario, sc_timepart now waits until the retired shard is absent from sqlite_master on every node.

This separates the basic failover/resume regression from the retired-shard overlap.

A separate test intentionally exercises the overlap and verifies that the cluster remains consistent and does not crash.

---

Additional memory-safety bugs found while testing

Running the new paths under ASAN exposed three pre-existing memory errors.

Dangling timepartition_name during replicant view reload

A table that had recently rolled out of a time partition could still have:

dbtable->timepartition_name

pointing at the old view's name after the view was freed.

If that replicant later became master, resume_schema_change() could read the dangling pointer while grouping shards by partition.

Fix

Before freeing the old view, views_handle_replicant_reload() now clears timepartition_name on any table still pointing at that view name.

---

free_sc() read the schema-change object after freeing it

The old code effectively did:

free_schema_change_type(s);

if (s->views_locked)
    ...

Fix

Save the value before the free:

int views_locked = s->views_locked;

free_schema_change_type(s);

if (views_locked)
    ...

---

llmeta list printed a non-NUL-terminated value with %s

LLMETA_TABLE_PARAMETERS values have a known length but are not guaranteed to be NUL-terminated.

Printing them with %s could read past the end of the buffer.

Fix

The value is now printed with a bounded length:

"%.*s", datalen, data

---

Testing

sc_seed_faults

A new deterministic test exercises the seed lifecycle through test-only fault injection.

Seed write deadlocks

Inject:

attempt 1 -> BDBERR_DEADLOCK
attempt 2 -> BDBERR_DEADLOCK
attempt 3 -> success

Verify:

• the ALTER succeeds;
• both injected deadlocks occurred;
• no stale seed remains;
• no in-schema-change marker remains.

Permanent seed-write failure

Inject a permanent seed-write error.

Verify for both single-table and multi-table schema changes:

• the ALTER fails;
• the requested schema is not applied;
• no seed remains;
• no in-schema-change marker remains;
• after disabling the injection, the same ALTER succeeds normally.

Seed-delete deadlocks

Force a schema change to fail and inject two deadlocks while its seed is being removed.

Verify:

• both delete deadlocks occur;
• the delete retries succeed;
• the failed schema change leaves no stale seed or marker.

---

sc_timepart_resume

A new cluster-only test directly exercises the time-partition resume lifecycle.

Failed resume modes

The test can force one shard to fail resume in four different ways:

1. seed fetch error;
2. missing seed/host;
3. master downgrading;
4. schema change already running / cannot set running.

For each failure:

• the injection is verified to have executed;
• the cluster must remain alive;
• the partition ALTER must abort consistently;
• no subset of shards may receive the new schema;
• stale in-schema-change markers must not remain, except where another schema change intentionally owns the table.

The consistency rule is:

0 shards changed
    OR
all shards changed

never only some shards

A normal partition ALTER is then run to verify that failed recovery attempts did not leave the partition unusable.

---

Double failover

The test also exercises:

sequenceDiagram
    participant A as Master A
    participant B as Master B
    participant C as Master C

    A->>A: Start time-partition ALTER
    Note over A: Conversion is still running

    A-->>B: A is killed
    B->>B: Win election
    B->>B: Begin resuming shard SCs

    Note over B: Test waits until resume is observed

    B-->>C: B is killed while resuming
    C->>C: Win election
    C->>C: Recover remaining SC state

    Note over C: Final shard schemas must be consistent

Loading

This verifies that recovery state remains valid even when the recovering master itself dies.

---

Retired-shard overlap

A separate case deliberately starts an ALTER while the shard rolled out of the partition is still being dropped.

The operation may complete or abort, but it must:

• not crash the database;
• not leave a partial schema across the current partition shards;
• leave the cluster operational.

---

Test-only fault injection

The new tests use COMDB2_TEST/internal tunables:

debug_sc_seed_set_fail
debug_sc_seed_delete_fail
debug_sc_resume_fail_table
debug_sc_resume_fail_mode

For seed operations:

N > 0
    fail the next N internally-owned operations with BDBERR_DEADLOCK

N < 0
    fail every operation with a non-deadlock error

Resume modes allow deterministic testing of the important start_schema_change() failure paths without relying on timing.

---

Validation

The final branch was validated with:

• seed stress: 30 tables across 40 multi-table transactions on a 3-node cluster;
• 1230 seed saves, 0 failures;
• sc_seed_faults;
• sc_timepart_resume;
• existing sc_timepart;
• ASAN with COMDB2_PER_THREAD_MALLOC=OFF.

After the memory fixes above, all three tests pass under ASAN with no ASAN reports.

As a negative control, restoring the old:

sc->sc_rc = SC_ABORTED;

behavior in verify_sc_resumed_for_all_shards() causes the new resume test to report the expected heap-use-after-free under ASAN.

This confirms that the new test directly exercises the ownership bug rather than only covering the surrounding failover scenario.

---

Result

Before this change, a master failure could expose several inconsistent states:

table marked in SC but seed missing

failed resume object already freed but still on resume list

shard failed to restart but remained marked in SC

stale seed left behind after cleanup deadlock

After this change:

flowchart TD
    A["Start shard schema change"] --> B["Persist required recovery metadata"]
    B --> C{"Successful?"}

    C -->|No| D["Fail startup cleanly"]
    D --> E["Do not publish unrecoverable SC state"]

    C -->|Yes| F["Run schema change"]

    F --> G{"Master survives?"}

    G -->|Yes| H["Complete and remove recovery metadata"]

    G -->|No| I["New master reconstructs shard SCs"]
    I --> J{"Each shard restarts?"}

    J -->|Yes| K["Resume coordinated partition ALTER"]

    J -->|No| L["Replace failed SC with safe placeholder"]
    L --> M["Abort coordinated partition ALTER"]
    M --> N["Clean stale recovery markers"]

Loading

The time-partition ALTER can therefore either resume successfully or abort as one coordinated operation without relying on missing recovery metadata or retaining freed schema-change objects.

[roborivers]

Cbuild submission: Success ✓.
Regression testing: Success ✓.

The first 10 failing tests are:
truncatesc_offline_generated **quarantined**
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**
phys_rep_tiered_firstfile_generated [timeout]
sql_logfill_autodisable [timeout]

[roborivers]

Cbuild submission: Success ✓.
Regression testing: Success ✓.

The first 10 failing tests are:
logfill [db unavailable at finish] **quarantined**
sc_truncate_multiddl_generated [db unavailable at finish] **quarantined**
sc_truncate [db unavailable at finish]
consumer_non_atomic_default_consumer_generated **quarantined**