fix(rbac): access control[manual-only] #35213
Conversation
kailixu
requested review from
a team,
dapan1121 and
guanshengliang
as code owners
kailixu
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the database privilege check logic to distinguish between rollup and trim operations and enhances the test suite with RSMA creation and rollup privilege tests. However, a large block of existing test cases was inadvertently commented out, which should be restored, and a leftover comment string needs to be removed for code cleanliness.
There was a problem hiding this comment.
Pull request overview
This PR adjusts RBAC enforcement for ROLLUP DATABASE by mapping the privilege check to a dedicated rollup operation (instead of treating it like TRIM DATABASE), and updates the privilege test suite to set up RSMA prerequisites for rollup scenarios.
Changes:
- Update
mndProcessTrimDbReq()to checkMND_OPER_ROLLUP_DBwhentrimReq.optrType == TSDB_OPTR_ROLLUP. - Extend the rollup privilege test to create a stable + RSMA before running
ROLLUP DATABASE. - Comment out large portions of the privilege control test suite (table/row/column/RBAC/system/etc. sections).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
test/cases/25-Privileges/test_priv_control.py |
Adds RSMA creation for rollup testing, but also disables many test sections by commenting them out. |
source/dnode/mnode/impl/src/mndDb.c |
Routes DB privilege checks to ROLLUP vs TRIM based on STrimDbReq.optrType. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Test: user can rollup database with privilege | ||
| self.login(user, pwd) | ||
| '''BUG20 | ||
| '''BUG20 ''' |
There was a problem hiding this comment.
'''BUG20 ''' is a standalone triple-quoted string statement. If this is meant to be a comment/marker, prefer a normal # comment (or a tracked TODO with issue ID) to avoid leaving an unused runtime string literal in the test body.
| '''BUG20 ''' | |
| # BUG20 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
source/libs/executor/src/sysscanoperator.c:5248
doDestroySysTableScanInfounconditionally callstsem_destroy(&pInfo->ready), butcreateSysTableScanOperatorInfoonly callstsem_init(&pInfo->ready, ...)in the non-local-scan path. For the local-scan cases (TABLES,TAGS,FILESETS),readyis never initialized, so destroying it can fail (and is undefined depending on platform semaphore implementation).
Consider initializing ready for all paths, or guard tsem_destroy behind a flag / pInfo->self > 0 / pInfo->readHandle.mnd != NULL as appropriate.
SSysTableScanInfo* pInfo = (SSysTableScanInfo*)param;
int32_t code = tsem_destroy(&pInfo->ready);
if (code != TSDB_CODE_SUCCESS) {
qError("%s failed at line %d since %s", __func__, __LINE__, tstrerror(code));
}
blockDataDestroy(pInfo->pRes);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Block this worker thread until the response arrives. qSemWait notifies | ||
| // the worker pool and waits, then re-acquires on wake-up. | ||
| code = qSemWait((qTaskInfo_t)pTaskInfo, &pInfo->ready); | ||
| if (code != TSDB_CODE_SUCCESS) { | ||
| freeSysTableLoadCtx(pLoadCtx); | ||
| qError("%s failed at line %d since %s", __func__, __LINE__, tstrerror(code)); | ||
| qError("%s tsem_wait failed at line %d since %s", __func__, __LINE__, tstrerror(code)); | ||
| pTaskInfo->code = code; | ||
| T_LONG_JMP(pTaskInfo->env, code); | ||
| } |
There was a problem hiding this comment.
sysTableScanFromMNode previously used tsem_timewait(..., VTB_REF_RPC_TIMEOUT_MS) but now blocks with qSemWait(..., &pInfo->ready) which has no timeout (it wraps tsem_wait). If the RPC callback is never invoked (transport error, dropped response, cancellation edge case), this worker thread can block indefinitely.
If a timeout is still required, consider adding a timed wait variant (e.g., qSemTimeWait) or restoring tsem_timewait and handling TSDB_CODE_TIMEOUT_ERROR explicitly.
| # Database privilege tests | ||
| print("[Database Privileges]") | ||
| self.create_snode() | ||
| self.create_qnode() | ||
| self.do_create_database_privilege() | ||
| self.do_alter_database_privilege() | ||
| self.do_drop_database_privilege() | ||
| self.do_use_database_privilege() | ||
| self.do_show_databases_privilege() | ||
| self.do_show_create_database_privilege() | ||
| self.do_flush_database_privilege() | ||
| self.do_compact_database_privilege() | ||
| self.do_trim_database_privilege() | ||
| self.do_rollup_database_privilege() | ||
| self.do_scan_database_privilege() | ||
| self.do_ssmigrate_database_privilege() | ||
|
|
||
| # Table privilege tests | ||
| print("") | ||
| print("[Table Privileges]") | ||
| self.do_create_table_privilege() | ||
| self.do_drop_table_privilege() | ||
| self.do_alter_table_privilege() | ||
| self.do_select_privilege() | ||
| self.do_insert_privilege() | ||
| self.do_delete_privilege() | ||
| self.do_select_column_privilege_comprehensive() | ||
| self.do_insert_column_privilege_comprehensive() | ||
| self.do_show_create_table_privilege() | ||
|
|
||
| # Column and row privilege tests | ||
| print("") | ||
| print("[Column and Row Privileges]") | ||
| self.do_row_privilege_with_tag_condition() | ||
| self.do_row_privilege_complex_conditions() | ||
| self.do_row_privilege_time_range() | ||
| self.do_row_privilege_mixed_conditions() | ||
| self.do_column_privilege() | ||
| self.do_column_mask_privilege() | ||
| self.do_column_row_combined_privilege() | ||
| self.do_column_privilege_update_priority() | ||
| self.do_privilege_update_time_priority() | ||
|
|
||
| # RBAC tests | ||
| print("") | ||
| print("[Role-Based Access Control]") | ||
| self.do_role_privilege() | ||
| self.do_role_creation_and_grant() | ||
| #self.do_role_lock_unlock() #can cause core BUG21 | ||
| self.do_system_roles() | ||
| self.do_audit_database_privileges() | ||
|
|
||
| # System privilege tests | ||
| print("") | ||
| print("[System Privileges]") | ||
| self.do_user_management_privileges() | ||
| self.do_token_management_privileges() | ||
| self.do_totp_management_privileges() | ||
| self.do_password_management_privileges() | ||
| self.do_node_management_privileges() | ||
| self.do_mount_management_privileges() | ||
| # self.do_create_database_privilege() | ||
| # self.do_alter_database_privilege() | ||
| # self.do_drop_database_privilege() | ||
| # self.do_use_database_privilege() | ||
| # self.do_show_databases_privilege() | ||
| # self.do_show_create_database_privilege() | ||
| # self.do_flush_database_privilege() | ||
| # self.do_compact_database_privilege() | ||
| # self.do_trim_database_privilege() | ||
| # self.do_rollup_database_privilege() | ||
| # self.do_scan_database_privilege() | ||
| # self.do_ssmigrate_database_privilege() | ||
|
|
||
| # # # Table privilege tests | ||
| # print("") | ||
| # print("[Table Privileges]") | ||
| # self.do_create_table_privilege() | ||
| # self.do_drop_table_privilege() | ||
| # self.do_alter_table_privilege() | ||
| # self.do_select_privilege() | ||
| # self.do_insert_privilege() | ||
| # self.do_delete_privilege() | ||
| # self.do_select_column_privilege_comprehensive() | ||
| # self.do_insert_column_privilege_comprehensive() | ||
| # self.do_show_create_table_privilege() | ||
|
|
||
| # # # Column and row privilege tests | ||
| # print("") | ||
| # print("[Column and Row Privileges]") | ||
| # self.do_row_privilege_with_tag_condition() | ||
| # self.do_row_privilege_complex_conditions() | ||
| # self.do_row_privilege_time_range() | ||
| # self.do_row_privilege_mixed_conditions() | ||
| # self.do_column_privilege() | ||
| # self.do_column_mask_privilege() | ||
| # self.do_column_row_combined_privilege() | ||
| # self.do_column_privilege_update_priority() | ||
| # self.do_privilege_update_time_priority() | ||
|
|
||
| # # # RBAC tests | ||
| # print("") | ||
| # print("[Role-Based Access Control]") | ||
| # self.do_role_privilege() | ||
| # self.do_role_creation_and_grant() | ||
| # #self.do_role_lock_unlock() #can cause core BUG21 | ||
| # self.do_system_roles() | ||
| # self.do_audit_database_privileges() | ||
|
|
||
| # # # System privilege tests | ||
| # print("") | ||
| # print("[System Privileges]") | ||
| # self.do_user_management_privileges() | ||
| # self.do_token_management_privileges() | ||
| # self.do_totp_management_privileges() | ||
| # self.do_password_management_privileges() | ||
| # self.do_node_management_privileges() | ||
| # self.do_mount_management_privileges() | ||
| self.do_system_variable_privileges() | ||
| self.do_information_schema_privileges() | ||
| self.do_system_monitoring_privileges() | ||
| self.do_show_grants_cluster_apps_privileges() | ||
| self.do_privilege_delegation() | ||
|
|
||
| # Function/index/tsrma/rsma privilege tests | ||
| print("") | ||
| print("[Function and Index Privileges]") | ||
| self.do_create_function_privilege() | ||
| self.do_create_index_privilege() | ||
| if platform.system().lower() != 'windows': | ||
| # windows does not support tsma | ||
| self.do_create_tsma_privilege() | ||
| self.do_create_rsma_privilege() | ||
| # self.do_information_schema_privileges() | ||
| # self.do_system_monitoring_privileges() | ||
| # self.do_show_grants_cluster_apps_privileges() | ||
| # self.do_privilege_delegation() | ||
|
|
||
| # # # Function/index/tsrma/rsma privilege tests | ||
| # print("") | ||
| # print("[Function and Index Privileges]") | ||
| # self.do_create_function_privilege() | ||
| # self.do_create_index_privilege() | ||
| # if platform.system().lower() != 'windows': | ||
| # # windows does not support tsma | ||
| # self.do_create_tsma_privilege() | ||
| # self.do_create_rsma_privilege() | ||
|
|
There was a problem hiding this comment.
test_priv_control() now comments out the majority of the privilege test suite (database/table/RBAC/system/etc.) and effectively only runs do_system_variable_privileges(). This significantly reduces automated coverage for RBAC/privilege behavior and makes regressions much harder to catch.
If the intent is "manual-only", consider gating the full suite behind an explicit flag (env var / CLI option) or marking individual tests as skipped, rather than commenting out the calls in the main test entrypoint.
| self.exec_sql_failed("SHOW QUERIES", TSDB_CODE_PAR_PERMISSION_DENIED) | ||
| '''BUG17 | ||
| '''BUG17''' | ||
| self.exec_sql_failed("SHOW CONNECTIONS", TSDB_CODE_PAR_PERMISSION_DENIED) | ||
| ''' | ||
|
|
There was a problem hiding this comment.
do_system_monitoring_privileges still has inconsistent handling for the SHOW CONNECTIONS negative check: the earlier "without privilege" assertion is commented out via a triple-quoted block, but after revoke the assertion is executed (the '''BUG17''' line is now just a no-op string). This makes the test coverage uneven and the intent unclear.
Consider removing the triple-quote blocks entirely and using a normal comment/skip mechanism so SHOW CONNECTIONS is either tested in both places or explicitly skipped in both.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
source/libs/executor/src/sysscanoperator.c:5247
doDestroySysTableScanInfo()always callstsem_destroy(&pInfo->ready), butpInfo->readyis only initialized viatsem_init()in the remote/MNode path. For local-scan operators (e.g. TABLES/TAGS/FILESETS), this may destroy an uninitialized semaphore and can crash or return undefined errors. Track initialization (e.g. areadyInitedflag) and only destroy when initialized, or initializereadyfor all code paths.
SSysTableScanInfo* pInfo = (SSysTableScanInfo*)param;
int32_t code = tsem_destroy(&pInfo->ready);
if (code != TSDB_CODE_SUCCESS) {
qError("%s failed at line %d since %s", __func__, __LINE__, tstrerror(code));
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # self.do_create_database_privilege() | ||
| # self.do_alter_database_privilege() | ||
| # self.do_drop_database_privilege() | ||
| # self.do_use_database_privilege() | ||
| # self.do_show_databases_privilege() | ||
| # self.do_show_create_database_privilege() | ||
| # self.do_flush_database_privilege() | ||
| # self.do_compact_database_privilege() | ||
| # self.do_trim_database_privilege() | ||
| # self.do_rollup_database_privilege() | ||
| # self.do_scan_database_privilege() | ||
| # self.do_ssmigrate_database_privilege() | ||
|
|
||
| # # # Table privilege tests | ||
| # print("") | ||
| # print("[Table Privileges]") | ||
| # self.do_create_table_privilege() | ||
| # self.do_drop_table_privilege() | ||
| # self.do_alter_table_privilege() | ||
| # self.do_select_privilege() | ||
| # self.do_insert_privilege() | ||
| # self.do_delete_privilege() | ||
| # self.do_select_column_privilege_comprehensive() | ||
| # self.do_insert_column_privilege_comprehensive() | ||
| # self.do_show_create_table_privilege() |
There was a problem hiding this comment.
test_priv_control() now comments out almost the entire privilege test suite (DB/Table/RBAC/System/etc.) and effectively only runs do_information_schema_privileges(). This will drastically reduce regression detection in CI; please avoid disabling tests by commenting out calls—use the framework’s skip/mark mechanism or gate the “manual-only” subset behind an explicit runtime flag/env var while keeping the default run comprehensive.
| # self.do_create_database_privilege() | |
| # self.do_alter_database_privilege() | |
| # self.do_drop_database_privilege() | |
| # self.do_use_database_privilege() | |
| # self.do_show_databases_privilege() | |
| # self.do_show_create_database_privilege() | |
| # self.do_flush_database_privilege() | |
| # self.do_compact_database_privilege() | |
| # self.do_trim_database_privilege() | |
| # self.do_rollup_database_privilege() | |
| # self.do_scan_database_privilege() | |
| # self.do_ssmigrate_database_privilege() | |
| # # # Table privilege tests | |
| # print("") | |
| # print("[Table Privileges]") | |
| # self.do_create_table_privilege() | |
| # self.do_drop_table_privilege() | |
| # self.do_alter_table_privilege() | |
| # self.do_select_privilege() | |
| # self.do_insert_privilege() | |
| # self.do_delete_privilege() | |
| # self.do_select_column_privilege_comprehensive() | |
| # self.do_insert_column_privilege_comprehensive() | |
| # self.do_show_create_table_privilege() | |
| self.do_create_database_privilege() | |
| self.do_alter_database_privilege() | |
| self.do_drop_database_privilege() | |
| self.do_use_database_privilege() | |
| self.do_show_databases_privilege() | |
| self.do_show_create_database_privilege() | |
| self.do_flush_database_privilege() | |
| self.do_compact_database_privilege() | |
| self.do_trim_database_privilege() | |
| self.do_rollup_database_privilege() | |
| self.do_scan_database_privilege() | |
| self.do_ssmigrate_database_privilege() | |
| # # # Table privilege tests | |
| print("") | |
| print("[Table Privileges]") | |
| self.do_create_table_privilege() | |
| self.do_drop_table_privilege() | |
| self.do_alter_table_privilege() | |
| self.do_select_privilege() | |
| self.do_insert_privilege() | |
| self.do_delete_privilege() | |
| self.do_select_column_privilege_comprehensive() | |
| self.do_insert_column_privilege_comprehensive() | |
| self.do_show_create_table_privilege() |
| // Fast path: if all sys-table priv bits are set, any table is accessible. | ||
| // privInfo bits 3-8: privInfoBasic|privInfoPrivileged|privInfoAudit|privInfoSec|privPerfBasic|privPerfPrivileged | ||
| if ((pParCxt->privInfo & 0x01F8u) == 0x01F8u) return 0; |
There was a problem hiding this comment.
transCheckSysTablePriv() uses a hard-coded bitmask (0x01F8u) for sys-table privilege bits. This is brittle and easy to break if the bit layout changes; please replace it with named constants/macros (or compute from the actual bitfield definitions) so the intent and maintenance contract are explicit.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
source/libs/parser/src/parTranslater.c:14666
- In translateCheckUserOptsPriv(), the password privilege branch compares
authRsp.userwithpParCxt->pUser, which will always match becausecatalogGetUserAuth()was requested forpParCxt->pUser. This makes the code always enforcePRIV_PASS_ALTER_SELFeven when changing another user’s password, and also leavestargetUserunused. ComparetargetUseragainst the current user (orpParCxt->pUser) to decide betweenPRIV_PASS_ALTERvsPRIV_PASS_ALTER_SELF.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # self.do_create_database_privilege() | ||
| # self.do_alter_database_privilege() | ||
| # self.do_drop_database_privilege() | ||
| # self.do_use_database_privilege() | ||
| # self.do_show_databases_privilege() | ||
| # self.do_show_create_database_privilege() | ||
| # self.do_flush_database_privilege() | ||
| # self.do_compact_database_privilege() |
There was a problem hiding this comment.
test_priv_control() now comments out almost the entire privilege test suite (DB/table/RBAC/system/function/view/stream/etc.) while the docstring and labels still describe it as a comprehensive CI test. This effectively removes coverage for most privilege checks and could let RBAC regressions slip through CI. If the intent is to make this "manual-only", consider gating sections behind an explicit flag/environment variable or splitting a dedicated, smaller manual test case instead of permanently commenting out the calls.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
source/libs/parser/src/parTranslater.c:14660
- The password-change privilege check compares
authRsp.userwithpParCxt->pUser, which are both the current user, so it will always take the “self password” path even when altering another user’s password.targetUseris computed but never used. Compare the target username being altered against the current user to decide betweenPRIV_PASS_ALTERvsPRIV_PASS_ALTER_SELF.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tdLog.info(f"api path: {apiPath}") | ||
| if platform.system().lower() == 'linux': | ||
| p = subprocess.Popen(f"cd {apiPath} && make", shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) | ||
| p = subprocess.Popen(f"cd {apiPath} && make -f make_partial", shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) |
There was a problem hiding this comment.
On Linux this invokes make -f make_partial, but the added file in script/api/ is named makefile_partial. As-is the build step will fail with “No rule to make target 'make_partial'”. Update the filename (or add the expected makefile) so the test can compile passwdTest.c reliably.
| p = subprocess.Popen(f"cd {apiPath} && make -f make_partial", shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) | |
| p = subprocess.Popen(f"cd {apiPath} && make -f makefile_partial", shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| p = subprocess.Popen(f"cd {apiPath} && make -f make_partial", shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) | ||
| out, err = p.communicate() | ||
| if 0 != p.returncode: | ||
| tdLog.exit("Test script passwdTest.c make failed") | ||
| else: | ||
| p = subprocess.Popen(f"cd {apiPath} && jom -f makefile_win64.mak", shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) | ||
| p = subprocess.Popen(f"cd {apiPath} && jom -f makefile_partial_win64.mak", shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) |
There was a problem hiding this comment.
Linux build path: make -f make_partial refers to a non-existent makefile in script/api (the added file is makefile_partial). This will fail the passwd test on Linux unless there is an untracked make_partial file in the environment. Update the command to point at the actual filename (or rename the makefile to match).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 29 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
source/libs/parser/src/parTranslater.c:14666
- In
translateCheckUserOptsPriv(), the password privilege check comparesauthRsp.userwithpParCxt->pUser, which are effectively the same current user, so the code will always take the “own password” branch and requirePRIV_PASS_ALTER_SELFeven when altering another user.targetUseris computed but never used. Please compare the current user (pParCxt->pUser) to the target user being altered/created (targetUser) to decide betweenPRIV_PASS_ALTERvsPRIV_PASS_ALTER_SELF.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 29 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
source/libs/parser/src/parTranslater.c:14633
- The password privilege check is comparing
authRsp.useragainstpParCxt->pUser, which (givencatalogGetUserAuth(..., pParCxt->pUser, &authRsp)) will always match the current user. This makes ALTER USER password changes always follow the “change own password” branch and can wrongly deny admins changing other users’ passwords. UsetargetUservs the current user to decide whether to requirePRIV_PASS_ALTERorPRIV_PASS_ALTER_SELF(and remove the now-unused/incorrect comparison).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 30 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
source/libs/parser/src/parTranslater.c:14633
translateCheckUserOptsPriv()calculatestargetUserbut then comparesauthRsp.uservspParCxt->pUser, which are both the current user returned bycatalogGetUserAuth(). This makes the password privilege check always take the “self” branch, so changing another user’s password (and CREATE USER with a password) is validated againstPRIV_PASS_ALTER_SELFinstead ofPRIV_PASS_ALTER. ComparetargetUserwith the current user (e.g.,pParCxt->pUser) to decide whether this is a self-password change.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (code != 0) { | ||
| mError("failed to get show variables info since %s", tstrerror(code)); | ||
| } | ||
| mndReleaseUser(pMnode, pUser); |
There was a problem hiding this comment.
mndReleaseUser(pMnode, pUser) is called unconditionally, but pUser can still be NULL on error paths before mndAcquireUser() succeeds. mndReleaseUser() does not handle NULL (it calls sdbRelease directly), so this can crash. Guard the release (e.g., if (pUser) ...) similarly to other mnode codepaths.
| mndReleaseUser(pMnode, pUser); | |
| if (pUser != NULL) { | |
| mndReleaseUser(pMnode, pUser); | |
| } |
| self.do_role_privilege() | ||
| self.do_role_creation_and_grant() | ||
| #self.do_role_lock_unlock() #can cause core BUG21 | ||
| self.do_role_lock_unlock() #can cause core BUG21 |
There was a problem hiding this comment.
This test is now unconditionally running do_role_lock_unlock() while the inline comment still says it “can cause core BUG21”. If the crash is still reproducible, this will make CI flaky; if it’s fixed, please remove/update the comment (or gate the test by version/feature flag) so it doesn’t signal known instability.
| self.do_role_lock_unlock() #can cause core BUG21 | |
| # Historically this test could trigger core BUG21, so keep it opt-in | |
| # until it is confirmed safe across supported environments. | |
| if os.environ.get("RUN_ROLE_LOCK_UNLOCK_TEST", "").lower() in ("1", "true", "yes", "on"): | |
| self.do_role_lock_unlock() |
Description
Issue(s)
Checklist
Please check the items in the checklist if applicable.