Skip to content

fix cldeadlock crashes - #6153

Open
markhannum wants to merge 1 commit into
bloomberg:mainfrom
markhannum:cldeadlock-fixes
Open

fix cldeadlock crashes#6153
markhannum wants to merge 1 commit into
bloomberg:mainfrom
markhannum:cldeadlock-fixes

Conversation

@markhannum

Copy link
Copy Markdown
Contributor

Fixes three crashes surfaced by running the cldeadlock test in a loop against a 3-node cluster.

Dangling clnt->thd. A pooled sql worker leaves clnt->thd pointing at its per-thread state after finishing a query; once signal_clnt_as_done hands the clnt back to the event thread, the watchdog's long-running-client scan (reqlog_long_running_clnt) can read clnt->thd while the worker thread is torn down, and recover_deadlock_flags could dereference it from the post-done flush path. The worker now detaches itself before signaling done via a new clnt_detach_thd helper: it clears sqlthd->clnt under gbl_sql_lock and clnt->thd under clnt->sql_lk (the lock the watchdog scan holds), and skips the detach on a nested replay since the outer frame still owns the thd. The watchdog scan also snapshots clnt->thd once instead of re-reading it, recover_deadlock_flags uses the TLS sql_thread instead of clnt->thd, and osql_clean_sqlclntstate preserves in_replay_nested across the osql bzero since it is call-stack state, not transaction state.

SEGV in newsql_write_postponed_evbuffer. run_stmt set postponed_write unconditionally, so if saving the postponed row failed (or produced no row), newsql_send_postponed_row later dereferenced a NULL/stale pointer. run_stmt now only claims a postponed row when send_row(postpone=1) actually succeeded, and newsql_send_postponed_row NULL-checks before sending.

Abort sending startgen with rqid 0. After a master swing, a commit retry could run osql_send_commit_logic on a session that had already been ended: the clnt_check_bdb_lock_desired path in osql_sock_start_int called osql_end even when OSQL_START_KEEP_RQID was set, unregistering the session and zeroing osql->rqid, and osql_sock_commit then retried the commit anyway, sending OSQL_STARTGEN with sid 0 and tripping the sid==0 assertion in osqlcomm_rpl_type_put. The lock-desired path now skips osql_end when keep_rqid is set (matching the existing post-send failure path), and osql_sock_commit only retries the commit logic when the session is still registered (osql->rqid != 0), otherwise failing the transaction so the client retries.

- dangling clnt->thd: worker now clears sqlthd->clnt / clnt->thd (under
  gbl_sql_lock / clnt->sql_lk) before signal_clnt_as_done; skipped on nested
  replay; watchdog long-running-clnt scan reads clnt->thd once
- sqlengine_work_lua_thread has the same clnt->thd lifecycle as
  sqlengine_work_appsock but was not detaching before signal_clnt_as_done;
  do it there too
- SEGV in newsql_write_postponed_evbuffer: only set postponed_write if the
  postponed row was actually saved; NULL-check before sending
- abort sending startgen with rqid 0: keep rqid on the lock-desired restart
  path when keep_rqid, and don't resend commit logic on an ended session

Signed-off-by: Mark Hannum <[email protected]>

@roborivers roborivers left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

The first 10 failing tests are:
sc_truncate_multiddl_generated [db unavailable at finish] **quarantined**
sc_resume_logicalsc_generated **quarantined**
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**

@roborivers roborivers left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

The first 10 failing tests are:
sc_resume_logicalsc_generated **quarantined**
ssl_san
consumer_non_atomic_default_consumer_generated **quarantined**
ssl_set_cmd
ssl_prefer
ssl_dbname
sc_downgrade [timeout] **quarantined**
queuedb_rollover_noroll1_generated [timeout] **quarantined**

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants