{180801974} Fix truncate stale cursor also fixing truncatesc rr test

[WalidNejmi]

Summary

Fix a crash when a running SQL statement spans an offline schema reload, and fix truncatesc so a real database crash is reported as a failure instead of eventually appearing as a 40-minute timeout.

There are two separate bugs:

1. Database bug: with online_recovery off, a log truncate that crosses a schema change can reload every table while a running SELECT has temporarily released its locks. The SELECT later resumes with cursors that still point at the old, freed dbtable / schema and closed BDB handles.

2. Test bug: truncatesc can call failexit from inside $(...). In that case, exit terminates only the command-substitution subshell, so the test keeps retrying after the physical replicant has already crashed and eventually hits the framework timeout.

This change:

• adds a generation counter to detect a wholesale schema reload while a statement's locks are released,

• fails the affected statement instead of letting it reuse stale cursors,

• clears stale cursor db and schema pointers before teardown,

• adds a deterministic regression test for the crash,

• strengthens the test to verify the overlapping SELECT actually fails,

• and makes truncatesc fail promptly when its physical replicant dies.

---

Background (only needed if unfamiliar)

A running SELECT has cursors that refer to both Comdb2-level and BDB-level state.

Conceptually:

BtCursor
   |
   +--> dbtable
   |      |
   |      +--> schema
   |      |
   |      +--> bdb_state
   |              |
   |              +--> BDB data/index handles
   |
   +--> schema pointer
   |
   +--> BDB cursor

Normally, the statement's table locks protect these objects.

When another thread needs the BDB writelock, a SELECT that is emitting rows can enter recover_deadlock().

That path temporarily gives up its BDB cursors and curtran, including its table locks, so the writer can proceed. Afterward it reacquires the transaction and attempts to continue.

That is safe only if the cursor's backing objects still exist.

A wholesale schema reload breaks that assumption.

---

Bug 1: stale cursors after an offline schema reload

With online_recovery off, a log truncate can undo a schema change.

During recovery this can call:

comdb2_reload_schemas()

which:

1. acquires table write locks,

2. closes the existing BDB handles,

3. frees the current dbtables and schemas,

4. reloads the tables from the recovered state,

5. opens replacement BDB handles.

The failure happens when a running SELECT has released its table locks immediately before this reload.

Failing sequence

Running SELECT
    |
    | cursor points at:
    |   old dbtable
    |   old schema
    |   old bdb_state
    |
    | holds table read locks
    v
BDB writer is requested

|

v
recover_deadlock()

|

| releases curtran

| releases table locks

|

| cursor still contains pointers to the old objects

v
offline recovery gets the writelock

|

v
comdb2_reload_schemas()

|

| closes old BDB handles

| frees old dbtables/schemas

| creates/reopens replacement objects

v
recover_deadlock()

|

| reacquires curtran/table locks

|

| those locks now protect the NEW table objects

v
SELECT resumes

|

| cursor still points at the OLD objects

v
bdb_cursor_next()

|

v
closed / NULL BDB handle

|

v
SIGSEGV

The reproduced crash reached:

get_cursor_for_cursortran_flags(db = NULL)

The stale cursor's old bdb_state had already been closed, while the live table had a different newly-opened bdb_state.

There is also a cleanup problem: even if the statement is aborted before the next cursor operation, its db and sc pointers can still refer to objects that were freed by the reload.

---

Why the existing table-version check is not enough

recover_deadlock() already has per-table schema/version checks.

That does not fully protect this case.

A wholesale reload can replace all of the in-memory objects without changing the logical table version.

For example:

before reload:

tableversion = 7

dbtable       = A

schema        = B

bdb_state     = C

after reload:

tableversion = 7

dbtable       = X

schema        = Y

bdb_state     = Z

A comparison like:

7 == 7

does not tell us that the cursor's pointers are still valid.

Worse, checking:

cur->db->tableversion

is itself unsafe once cur->db may already point to freed memory.

The property we need to detect is:

Were the in-memory table objects destroyed and recreated while this statement had no table locks?

---

Fix: detect a schema reload across the unlocked interval

Add a database-wide:

reload_schemas_gen

comdb2_reload_schemas() increments it after acquiring all table write locks and immediately before closing/freeing the current tables.

recover_deadlock() records the generation while it still owns its table locks.

The sequence is:

recover_deadlock()

|

| read generation = 25

|

| release table locks

v

comdb2_reload_schemas()

|

| acquire all table write locks

| generation: 25 -> 26

| close old handles

| free old dbtables/schemas

| load replacement tables

v
recover_deadlock()

|

| reacquire curtran/table locks

| read generation = 26

|

| 25 != 26

v
The statement crossed a wholesale schema reload.
Do not reuse its old cursor state.

Fail the statement instead.

If the generation did not change, the normal recover_deadlock() cursor recovery path continues as before.

If it did change, the old cursors cannot safely be repaired in place because they contain more state than just a dbtable *: BDB cursor state, schema-derived metadata, buffers, index state, and other cached information may all correspond to the old objects.

The safe behavior is to abort the statement and let the client retry.

---

Cursor cleanup

On a generation mismatch, clear both:

cur->db = NULL;

cur->sc = NULL;

for local cursors.

cur->db points at the old freed dbtable.

For table cursors, cur->sc points at a schema owned by that table and is also stale after the reload.

This was verified directly.

Before the generation-mismatch handling:

db = non-NULL

sc = non-NULL

With the original implementation:

db = NULL

sc = same old address

recover_deadlock_sc_cleanup() did not clear sc, because it only clears the cursor when cur->db is still non-NULL.

Instrumentation of schema destruction confirmed that the stale sc address matched the schema address that had been freed during the reload.

The current teardown path does not presently dereference that stale sc, so this was not a second reproduced crash. Clearing it restores the intended cursor invariant and prevents future cleanup code from observing freed schema memory.

The fix is kept local to this wholesale-reload path rather than changing the semantics of the shared cleanup helper for unrelated cursor types.

---

Bug 2: truncatesc turns a crash into a timeout

The existing test has paths like:

assert_select_all

-> select_all

-> assert_select_all_rep

-> failexit

that can execute inside command substitution:

x=$(...)

In Bash, command substitution runs in a subshell.

Therefore:

exit -1

inside failexit exits only that subshell.

The parent runit process keeps running.

The observed sequence was:

physical replicant crashes

|

v

failexit runs from inside $(...)

|

v

only the subshell exits

|

v

the parent test keeps retrying

|

v

same failure repeats

|

v

framework reaches 40-minute timeout

|

v

reported as [timeout]

This masked the actual database crash.

Fix

failexit now:

• writes its message to stderr, so command substitution does not swallow it,

• terminates the top-level runit shell as well,

• keeps the existing .failexit marker behavior.

This was verified by deliberately killing the physical replicant at the same point.

Before the fix:

physrep dies

failexit runs repeatedly

runit remains alive

framework eventually reports timeout

After the fix:

physrep dies

failexit message is visible

runit terminates

framework reports a real test failure

---

Reader behavior in truncatesc

This production fix intentionally changes what happens to a reader that spans an offline schema reload.

Previously:

reader resumes with stale state

-> may crash the process

Now:

reader detects wholesale reload

-> statement fails

Because the test deliberately causes these reloads, its background reader_thread can now see expected transient read failures.

The reader error threshold therefore counts consecutive errors rather than accumulating every isolated error across the entire test.

This was measured during stress testing:

64 total reader failures observed

64/64 = "Timeout while reading response from server"

64/64 occurred 0-6 seconds after a schema reload

successful reads occurred between failures

No unrelated reader errors were observed in those runs.

---

New deterministic regression test

Add:

truncate_stale_cursor.test

The old failure depended heavily on timing and could take a long time to reproduce.

The new test deliberately constructs the unsafe sequence.

Setup

Create t1 and insert 60 rows:

t1 = 60 rows

Record the current LSN.

Then do additional work after that LSN:

insert another 60 rows into t1

create table t2

Immediately before truncation:

t1 = 120 rows

t2 exists

The saved LSN represents:

t1 = 60 rows

t2 does not exist

Force a SELECT to be active during the truncate

Set:

sql_row_delay_msecs = 1000

and run:

select * from t1;

on every node.

The test does not rely on a fixed sleep.

For every SELECT it waits until:

at least one row has been returned

AND

the process is still alive

Only then does the test issue the log truncate.

This proves that the SELECT is actually mid-scan when recovery starts.

Truncate

Truncate back to the saved LSN.

This should:

remove the second 60 rows

undo CREATE TABLE t2

trigger the schema reload

Assertions

For every overlapping SELECT:

it must have been alive before the truncate

AND
it must NOT successfully complete across the schema reload

The exact client error text is intentionally not checked.

The current client path usually reports:

Timeout while reading response from server

rather than the internal schema-change error.

Afterward, verify:

every database node is still alive

t1 contains exactly 60 rows

t2 no longer exists

So the test checks both sides of the fix:

cursor behavior:

stale SELECT fails

database behavior:

process survives

recovered data is correct

recovered schema is correct

---

A/B verification

The strengthened regression test was run against several configurations.

Unfixed main

Fails.

Observed behavior includes:

SELECT succeeds across the reload

as well as database cores.

Some replicant SELECTs returned all 120 rows with rc = 0, which the previous version of the test did not independently reject.

Original PR

The stale-cursor crash is prevented:

SELECT fails

server survives

but diagnostics showed cur->sc remained pointing at freed schema memory.

Final version

Passes in both cluster and single-node configurations.

All deliberately overlapping SELECTs terminate nonzero after the reload.

Negative control

The generation check was temporarily disabled on the fixed branch.

The strengthened test failed again and produced cores.

This verifies that the regression test is actually protecting the generation-based stale-cursor fix rather than merely passing because of timing differences.

---

Targeted validation

The final code passed the following targeted tests:

Test	Result
truncate_stale_cursor cluster	3/3 pass
truncate_stale_cursor single node	pass
truncatesc_offline_generated, 3 concurrent	3/3 pass, 0 cores
truncatesc	pass
recover_deadlock	pass
analyze_recover_deadlock	pass in cluster
cldeadlock	pass
commit_lsn_map	pass
snapshot_during_truncate	pass
temptable-truncate	pass
truncoplog	pass
truncstat	pass

sc_truncate timed out on both the baseline and fixed builds. The schema-reload path was not entered in either case, so that timeout is not attributed to this change.

The full Comdb2 regression suite was not run as part of the local investigation; normal CI/RoboMark coverage is still required.

---

Known follow-up

When a statement is aborted from this path while rows are already being emitted, the server internally detects the schema-change condition, but cdb2sql generally reports:

Timeout while reading response from server

after its read timeout rather than receiving a schema-change error response.

That behavior predates this fix and is not required for stale-cursor safety.

A follow-up should investigate propagating the actual schema-change error to the client so it can fail immediately instead of waiting for a read timeout.

[roborivers]

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

The first 10 failing tests are:
timepart_retro
queuedb_rollover_noroll1_generated **quarantined**
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**
reco-ddlk-sql [timeout] **quarantined**

[roborivers]

Cbuild submission: Success ✓.
Regression testing: 3/730 tests failed ⚠.

The first 10 failing tests are:
comdb2sys **quarantined**
bind_query_plan
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**
reco-ddlk-sql [timeout] **quarantined**