Claim exclusive ownership for concurrent rebase continue/abort - #2004
Merged
Conversation
added 2 commits
August 5, 2026 19:20
When --continue and --abort race, both could pass the in-session
isRebasing check and then both fail mid-cleanup ("rebase failed" /
"rebase recovery failed"), leaving recovery unpredictable.
Add rebaseClaimActiveEnd: under the graph lock, reload durable working
set state; if isRebasing is already clear return SQLITE_DONE ("no rebase
in progress"); otherwise clear the flag, persist it, and drop
dolt_rebase so only one side proceeds. Abort and continue both claim
before cleanup/replay. Make post-claim cleanup idempotent when the
temporary working branch is already gone, so a lost race reports
"no rebase in progress" instead of a stuck recovery failure.
Absent plan table also reports "no rebase in progress" for consistency.
Test: multi_process_merge_rebase_test races continue vs abort across
forked connections for 12 trials.
Checked builds use -Werror=unused-result; ignore-free write() on the child result pipe failed CI on Ubuntu.
Contributor
DoltLite source coverage
Merged 165 pooled raw profiles from the distributed Linux correctness jobs. Per-file coverage (91 files)
|
Contributor
DoltLite performance vs PR base
blobpk details
compositepk details
int details
textpk details
vc details
All relative performance gates passed. |
|
SummaryCoverage focused on safe completion and cancellation of repository rebase operations, including concurrent requests, stale connections, reopening, cleanup, error recovery, malformed state, and bounded retries. Both normal workflows and adversarial timing and fault conditions remained healthy, with the repository consistently usable for later commits. Safe to merge — the exercised rebase, cleanup, concurrency, persistence, and error-handling behaviors showed no PR-attributable regressions or failures. A small number of specialized fault scenarios were not exercised, but they are coverage caveats rather than merge blockers. Tests run by Ito
Tip Reply with @itoqa to send us feedback on this test run. |
Concurrent --continue can leave abort's best-effort cleanup racy after isRebasing is already cleared. Once the claim wins, report "Interactive rebase aborted" regardless of cleanup noise. On claim hard-failure, re-check durable state and prefer "no rebase in progress" when a peer already ended the rebase.
|
Diff SummaryCoverage spans normal interactive rebase completion and abort flows, durable state across reopen, concurrent terminal operations, cleanup safety, branch preservation, conflict handling, and adversarial storage-failure cases. The normal and concurrency behavior is broadly healthy, but recovery-error reporting remains unreliable when abort cleanup or state verification fails. Not safe to merge yet — this PR has multiple attributable regressions and new failures in abort recovery handling that can hide storage or state-refresh errors while leaving cleanup incomplete or the repository state uncertain. An unrelated pre-existing recovery-reporting issue is a flag for later, but the concentration of PR-related failures makes this a merge blocker. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Abort hides a rebase recovery failure
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
![itoqa[bot]](https://avatars.githubusercontent.com/in/2622614?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔁 Regression: previously passing at adbbb61
Abort hides cleanup failures
What failed: The injected cleanup error was reproduced by the native regression harness, but abort returned "Interactive rebase aborted" instead of the expected "rebase recovery failed" error. The harness recorded 11 passing checks and one failed assertion for this recovery-failure message.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: When a storage error interrupts abort cleanup, users are told the rebase was aborted successfully even though cleanup may be incomplete. They may need to retry or manually recover the repository state.
- Steps to Reproduce:
- Create a repository with main and feat branches, then start an interactive rebase on feat.
- Inject a non-SQLITE_NOTFOUND error while abort removes the temporary rebase branch.
- Run the interactive rebase abort operation and capture its result.
- Check the temporary branch and working-set state after the operation.
- Stub / mock content: The native regression harness used SQLite fault injection to simulate a non-NOTFOUND ref deletion error. No application routes, external services, or production data were used.
- Code Analysis: The cleanup helper rebaseCleanupAfterClaim in src/doltlite_rebase.c:1101-1127 preserves the first error from doltliteMutateRefs and doltlitePersistWorkingSet, so a ref deletion error other than SQLITE_NOTFOUND remains a failure. The only allowed idempotent exception is explicit in rebaseDeleteWorkingBranchRefs at src/doltlite_rebase.c:1021-1027, where SQLITE_NOTFOUND is converted to SQLITE_OK. In the PR-changed abort path at src/doltlite_rebase.c:1434-1442, the return value from rebaseCleanupAfterClaim is explicitly cast to void, and the return values from rebaseRestoreReturnBranchWorkingState, doltlitePersistWorkingSet, and doltliteVcSealBranchStyleTxn are also discarded before src/doltlite_rebase.c:1447 always returns "Interactive rebase aborted". The regression test in test/doltlite_regression_test_c.c:6948-6959 installs fault code 953 for the abort cleanup path and requires an ERROR: rebase recovery failed result; that assertion failed. The smallest practical fix is to retain the cleanup return code after the durable claim, return rebaseResultRecoveryFailure when a non-NOTFOUND cleanup error occurs, and continue treating only SQLITE_NOTFOUND as successful idempotent cleanup. The post-claim claim ownership should remain durable, so this fix only restores truthful error reporting rather than undoing the claim.
- Why this is likely a bug: This is not only a runtime symptom: the source records non-NOTFOUND cleanup errors, while the abort caller deliberately throws that result away and unconditionally emits a success message. The native fault-injection test directly exercises the error path and fails exactly where the public error contract requires recovery failure. A real I/O or storage error can therefore be hidden from the caller, leaving cleanup incomplete while presenting a successful terminal result. The PR diff directly introduced the discarded cleanup-result behavior at src/doltlite_rebase.c:1434-1442, so the smallest fix is to propagate that retained error while preserving the intended success behavior for a missing temporary ref.
Relevant code
src/doltlite_rebase.c:1021-1027
rc = chunkStoreDeleteBranch(cs, zWorkingBranch);
return rc==SQLITE_NOTFOUND ? SQLITE_OK : rc;src/doltlite_rebase.c:1104-1127
static int rebaseCleanupAfterClaim(
sqlite3 *db,
const char *zOrigBranch,
const char *zWorkingBranch
){
...
rebaseKeepFirstError(&rc, rc2);
...
return rc;
}src/doltlite_rebase.c:1434-1447
(void)rebaseCleanupAfterClaim(db, zOrigBranch, zWorking);
...
(void)doltlitePersistWorkingSet(db);
(void)doltliteVcSealBranchStyleTxn(db);
...
sqlite3_result_text(context, "Interactive rebase aborted", -1, SQLITE_STATIC);test/doltlite_regression_test_c.c:6948-6959
gRegressionFaultCode = 953;
...
res = queryScalarText(db, "SELECT dolt_rebase('--abort')");
...
check("rebase_abort_recovery_failure_is_returned",
strstr(res, "ERROR: rebase recovery failed")!=0);Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Abort hides cleanup failures**
**What failed:** The injected cleanup error was reproduced by the native regression harness, but abort returned "Interactive rebase aborted" instead of the expected "rebase recovery failed" error. The harness recorded 11 passing checks and one failed assertion for this recovery-failure message.
- **Impact:** When a storage error interrupts abort cleanup, users are told the rebase was aborted successfully even though cleanup may be incomplete. They may need to retry or manually recover the repository state.
- **Steps to reproduce:**
1. Create a repository with main and feat branches, then start an interactive rebase on feat.
2. Inject a non-SQLITE_NOTFOUND error while abort removes the temporary rebase branch.
3. Run the interactive rebase abort operation and capture its result.
4. Check the temporary branch and working-set state after the operation.
- **Stub / mock content:** The native regression harness used SQLite fault injection to simulate a non-NOTFOUND ref deletion error. No application routes, external services, or production data were used.
- **Code analysis:** The cleanup helper rebaseCleanupAfterClaim in src/doltlite_rebase.c:1101-1127 preserves the first error from doltliteMutateRefs and doltlitePersistWorkingSet, so a ref deletion error other than SQLITE_NOTFOUND remains a failure. The only allowed idempotent exception is explicit in rebaseDeleteWorkingBranchRefs at src/doltlite_rebase.c:1021-1027, where SQLITE_NOTFOUND is converted to SQLITE_OK. In the PR-changed abort path at src/doltlite_rebase.c:1434-1442, the return value from rebaseCleanupAfterClaim is explicitly cast to void, and the return values from rebaseRestoreReturnBranchWorkingState, doltlitePersistWorkingSet, and doltliteVcSealBranchStyleTxn are also discarded before src/doltlite_rebase.c:1447 always returns "Interactive rebase aborted". The regression test in test/doltlite_regression_test_c.c:6948-6959 installs fault code 953 for the abort cleanup path and requires an ERROR: rebase recovery failed result; that assertion failed. The smallest practical fix is to retain the cleanup return code after the durable claim, return rebaseResultRecoveryFailure when a non-NOTFOUND cleanup error occurs, and continue treating only SQLITE_NOTFOUND as successful idempotent cleanup. The post-claim claim ownership should remain durable, so this fix only restores truthful error reporting rather than undoing the claim.
- **Why this is likely a bug:** This is not only a runtime symptom: the source records non-NOTFOUND cleanup errors, while the abort caller deliberately throws that result away and unconditionally emits a success message. The native fault-injection test directly exercises the error path and fails exactly where the public error contract requires recovery failure. A real I/O or storage error can therefore be hidden from the caller, leaving cleanup incomplete while presenting a successful terminal result. The PR diff directly introduced the discarded cleanup-result behavior at src/doltlite_rebase.c:1434-1442, so the smallest fix is to propagate that retained error while preserving the intended success behavior for a missing temporary ref.
**Relevant code:**
`src/doltlite_rebase.c:1021-1027`
~~~c
rc = chunkStoreDeleteBranch(cs, zWorkingBranch);
return rc==SQLITE_NOTFOUND ? SQLITE_OK : rc;
~~~
`src/doltlite_rebase.c:1104-1127`
~~~c
static int rebaseCleanupAfterClaim(
sqlite3 *db,
const char *zOrigBranch,
const char *zWorkingBranch
){
...
rebaseKeepFirstError(&rc, rc2);
...
return rc;
}
~~~
`src/doltlite_rebase.c:1434-1447`
~~~c
(void)rebaseCleanupAfterClaim(db, zOrigBranch, zWorking);
...
(void)doltlitePersistWorkingSet(db);
(void)doltliteVcSealBranchStyleTxn(db);
...
sqlite3_result_text(context, "Interactive rebase aborted", -1, SQLITE_STATIC);
~~~
`test/doltlite_regression_test_c.c:6948-6959`
~~~c
gRegressionFaultCode = 953;
...
res = queryScalarText(db, "SELECT dolt_rebase('--abort')");
...
check("rebase_abort_recovery_failure_is_returned",
strstr(res, "ERROR: rebase recovery failed")!=0);
~~~
| goto claim_done; | ||
| } | ||
|
|
||
| rc = doltliteClearSessionRebaseState(db); |
There was a problem hiding this comment.
🔁 Regression: previously passing at adbbb61
Abort hides rebase recovery errors
What failed: The abort operation hit the injected storage failure, but the result did not contain the expected recovery error. The test's recovery-failure assertion failed because the operation instead treated the cleared durable state as proof that no rebase was in progress.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: When an abort encounters a storage failure, users are told that no rebase is in progress instead of being warned that recovery failed. Rebase state can remain behind, requiring manual recovery.
- Steps to Reproduce:
- Create a repository with main and feat branches, then start an interactive rebase from feat onto main.
- Inject storage fault 953 while the abort operation drops the rebase plan after clearing durable rebase state.
- Run the rebase abort operation and inspect its returned error text.
- Compare the result with the expected recovery-failure message and confirm it does not report successful abort completion.
- Stub / mock content: The test used an intentional native storage-fault injection to exercise recovery handling; no application mocks, route stubs, or source bypasses were applied.
- Code Analysis: In /tmp/output-agent-workspace/repo/src/doltlite_rebase.c, rebaseClaimActiveEnd() clears and persists the durable rebase state at lines 1070-1073, then calls sqlite3FaultSim(953) before DROP TABLE at lines 1080-1083. That makes fault 953 a claim-adjacent error: the rebase flag is already cleared, but removing main.dolt_rebase has failed. The PR diff changes doltliteRebaseInteractiveAbort() at lines 1413-1429. After any non-OK claim result, it force-refreshes and reloads the working set, then returns no rebase in progress when stillRebasing is false. Because fault 953 occurs after the durable flag was cleared, this new branch suppresses rebaseResultRecoveryFailure(context, rc), which is the required error path. The smallest fix is to distinguish a peer-lost claim (SQLITE_DONE) from a claim error after local durable termination began, or otherwise preserve the claim error for fault 953 instead of converting every refreshed clear flag into no rebase.
- Why this is likely a bug: The targeted native test passed 11 of 12 assertions and confirmed that fault 953 was injected; only the required recovery-error category failed. The source path explains the exact mismatch without relying on the unavailable browser service: the changed abort handler uses the post-failure cleared flag as a peer-loss signal even when this same operation cleared it and then failed to drop the plan. This can hide a real storage/recovery failure and leave main.dolt_rebase behind, so it is a production error-handling defect rather than test setup noise. A targeted fix should preserve recovery failure for errors returned after local durable claim work, while retaining no rebase in progress for a genuine SQLITE_DONE peer win.
Relevant code
src/doltlite_rebase.c:1070-1083
rc = doltliteClearSessionRebaseState(db); ... rc = doltliteSaveWorkingSet(db); ... rcDrop = sqlite3FaultSim(953) ? SQLITE_IOERR : sqlite3_exec(db, "DROP TABLE IF EXISTS main.dolt_rebase", 0, 0, 0);src/doltlite_rebase.c:1413-1429
if( rc!=SQLITE_OK ){ ... doltliteGetSessionRebaseState(db, &stillRebasing, 0, 0, 0, 0); ... if( !stillRebasing ) sqlite3_result_error(context, "no rebase in progress", -1); else rebaseResultRecoveryFailure(context, rc); }test/doltlite_regression_test_c.c:6948-6959
gRegressionFaultCode = 953; ... res = queryScalarText(db, "SELECT dolt_rebase('--abort')"); ... check("rebase_abort_recovery_failure_is_returned", strstr(res, "ERROR: rebase recovery failed")!=0);Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Abort hides rebase recovery errors**
**What failed:** The abort operation hit the injected storage failure, but the result did not contain the expected recovery error. The test's recovery-failure assertion failed because the operation instead treated the cleared durable state as proof that no rebase was in progress.
- **Impact:** When an abort encounters a storage failure, users are told that no rebase is in progress instead of being warned that recovery failed. Rebase state can remain behind, requiring manual recovery.
- **Steps to reproduce:**
1. Create a repository with main and feat branches, then start an interactive rebase from feat onto main.
2. Inject storage fault 953 while the abort operation drops the rebase plan after clearing durable rebase state.
3. Run the rebase abort operation and inspect its returned error text.
4. Compare the result with the expected recovery-failure message and confirm it does not report successful abort completion.
- **Stub / mock content:** The test used an intentional native storage-fault injection to exercise recovery handling; no application mocks, route stubs, or source bypasses were applied.
- **Code analysis:** In /tmp/output-agent-workspace/repo/src/doltlite_rebase.c, rebaseClaimActiveEnd() clears and persists the durable rebase state at lines 1070-1073, then calls sqlite3FaultSim(953) before DROP TABLE at lines 1080-1083. That makes fault 953 a claim-adjacent error: the rebase flag is already cleared, but removing main.dolt_rebase has failed. The PR diff changes doltliteRebaseInteractiveAbort() at lines 1413-1429. After any non-OK claim result, it force-refreshes and reloads the working set, then returns no rebase in progress when stillRebasing is false. Because fault 953 occurs after the durable flag was cleared, this new branch suppresses rebaseResultRecoveryFailure(context, rc), which is the required error path. The smallest fix is to distinguish a peer-lost claim (SQLITE_DONE) from a claim error after local durable termination began, or otherwise preserve the claim error for fault 953 instead of converting every refreshed clear flag into no rebase.
- **Why this is likely a bug:** The targeted native test passed 11 of 12 assertions and confirmed that fault 953 was injected; only the required recovery-error category failed. The source path explains the exact mismatch without relying on the unavailable browser service: the changed abort handler uses the post-failure cleared flag as a peer-loss signal even when this same operation cleared it and then failed to drop the plan. This can hide a real storage/recovery failure and leave main.dolt_rebase behind, so it is a production error-handling defect rather than test setup noise. A targeted fix should preserve recovery failure for errors returned after local durable claim work, while retaining no rebase in progress for a genuine SQLITE_DONE peer win.
**Relevant code:**
`src/doltlite_rebase.c:1070-1083`
~~~c
rc = doltliteClearSessionRebaseState(db); ... rc = doltliteSaveWorkingSet(db); ... rcDrop = sqlite3FaultSim(953) ? SQLITE_IOERR : sqlite3_exec(db, "DROP TABLE IF EXISTS main.dolt_rebase", 0, 0, 0);
~~~
`src/doltlite_rebase.c:1413-1429`
~~~c
if( rc!=SQLITE_OK ){ ... doltliteGetSessionRebaseState(db, &stillRebasing, 0, 0, 0, 0); ... if( !stillRebasing ) sqlite3_result_error(context, "no rebase in progress", -1); else rebaseResultRecoveryFailure(context, rc); }
~~~
`test/doltlite_regression_test_c.c:6948-6959`
~~~c
gRegressionFaultCode = 953; ... res = queryScalarText(db, "SELECT dolt_rebase('--abort')"); ... check("rebase_abort_recovery_failure_is_returned", strstr(res, "ERROR: rebase recovery failed")!=0);
~~~
| zBranch = doltliteGetSessionBranch(db); | ||
| if( !zBranch || !zBranch[0] ) zBranch = "main"; | ||
|
|
||
| rc = chunkStoreLockAndRefresh(cs); |
There was a problem hiding this comment.
🆕 New Failure: identified in this diff run
Abort hides refresh failures during recovery
What failed: The recovery regression completed 11 of 12 checks, but rebase_abort_recovery_failure_is_returned failed. The abort operation did not return the required 'ERROR: rebase recovery failed' result.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: When a rebase abort cannot verify durable state, users receive a generic failure instead of a recovery-failure message. They may not know that the rebase state is unverified and needs recovery guidance.
- Steps to Reproduce:
- Create a repository with main and feat branches and start an interactive rebase from feat onto main.
- Inject a non-BUSY claim error followed by a force-refresh or durable working-set reload failure.
- Run dolt_rebase('--abort').
- Check the returned error and the persisted isRebasing state.
- Stub / mock content: The test used controlled SQLite fault injection to model a hard claim and refresh failure. No production services, customer data, or application-route mocks were used.
- Code Analysis: The new claim path in rebaseClaimActiveEnd() calls chunkStoreLockAndRefresh(), chunkStoreForceRefresh(), and doltliteLoadWorkingSet() at src/doltlite_rebase.c:1057-1063. Any non-OK result exits before the durable claim completes. The caller then handles the claim result at src/doltlite_rebase.c:1404-1447, where the PR added a refresh-and-reload check before deciding whether the peer won; if those checks cannot read durable state, the code must retain the original claim error and call rebaseResultRecoveryFailure(), not downgrade the result to a generic rebase failure or infer that no rebase is active. The continue path also demonstrates the unsafe split at src/doltlite_rebase.c:1544-1550 and src/doltlite_rebase.c:1640-1647: bPlanDropped is set only after a successful claim, so a claim-side hard error reaches the pre-claim branch and returns 'rebase failed'. The smallest practical fix is to preserve the original claim error whenever refresh or working-set reload is unsuccessful, return the recovery-failure message, and only report 'no rebase in progress' after a successful durable-state read proves that isRebasing is clear.
- Why this is likely a bug: The failure is reproduced by a deterministic C regression with an injected claim/refresh fault, and the source contains the exact early-return and error-routing paths that produce the wrong contract. The expected behavior is explicit in the regression oracle: when durable state cannot be read, abort must report recovery failure and must not claim that a peer cleared the rebase. The PR's changed abort claim flow is the direct cause, so a targeted correction to its error decision is appropriate.
Relevant code
src/doltlite_rebase.c:1057-1063
rc = chunkStoreLockAndRefresh(cs);
if( rc!=SQLITE_OK ) return rc;
...
rc = chunkStoreForceRefresh(cs);
if( rc!=SQLITE_OK ) goto claim_done;
rc = doltliteLoadWorkingSet(db, zBranch);
if( rc!=SQLITE_OK ) goto claim_done;src/doltlite_rebase.c:1404-1447
rc = rebaseClaimActiveEndRetry(db);
...
if( rc!=SQLITE_OK ){
...
if( !stillRebasing ){
sqlite3_result_error(context, "no rebase in progress", -1);
}else{
rebaseResultRecoveryFailure(context, rc);
}
return;
}
...
sqlite3_result_text(context, "Interactive rebase aborted", -1, SQLITE_STATIC);src/doltlite_rebase.c:1544-1550
rc = rebaseClaimActiveEndRetry(db);
if( rc==SQLITE_DONE ){ ... }
if( rc!=SQLITE_OK ) goto abort_err;
bPlanDropped = 1;test/doltlite_regression_test_c.c:6948-6959
gRegressionFaultCode = 953;
...
res = queryScalarText(db, "SELECT dolt_rebase('--abort')");
...
check("rebase_abort_recovery_failure_is_returned",
strstr(res, "ERROR: rebase recovery failed")!=0);Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Abort hides refresh failures during recovery**
**What failed:** The recovery regression completed 11 of 12 checks, but rebase_abort_recovery_failure_is_returned failed. The abort operation did not return the required 'ERROR: rebase recovery failed' result.
- **Impact:** When a rebase abort cannot verify durable state, users receive a generic failure instead of a recovery-failure message. They may not know that the rebase state is unverified and needs recovery guidance.
- **Steps to reproduce:**
1. Create a repository with main and feat branches and start an interactive rebase from feat onto main.
2. Inject a non-BUSY claim error followed by a force-refresh or durable working-set reload failure.
3. Run dolt_rebase('--abort').
4. Check the returned error and the persisted isRebasing state.
- **Stub / mock content:** The test used controlled SQLite fault injection to model a hard claim and refresh failure. No production services, customer data, or application-route mocks were used.
- **Code analysis:** The new claim path in rebaseClaimActiveEnd() calls chunkStoreLockAndRefresh(), chunkStoreForceRefresh(), and doltliteLoadWorkingSet() at src/doltlite_rebase.c:1057-1063. Any non-OK result exits before the durable claim completes. The caller then handles the claim result at src/doltlite_rebase.c:1404-1447, where the PR added a refresh-and-reload check before deciding whether the peer won; if those checks cannot read durable state, the code must retain the original claim error and call rebaseResultRecoveryFailure(), not downgrade the result to a generic rebase failure or infer that no rebase is active. The continue path also demonstrates the unsafe split at src/doltlite_rebase.c:1544-1550 and src/doltlite_rebase.c:1640-1647: bPlanDropped is set only after a successful claim, so a claim-side hard error reaches the pre-claim branch and returns 'rebase failed'. The smallest practical fix is to preserve the original claim error whenever refresh or working-set reload is unsuccessful, return the recovery-failure message, and only report 'no rebase in progress' after a successful durable-state read proves that isRebasing is clear.
- **Why this is likely a bug:** The failure is reproduced by a deterministic C regression with an injected claim/refresh fault, and the source contains the exact early-return and error-routing paths that produce the wrong contract. The expected behavior is explicit in the regression oracle: when durable state cannot be read, abort must report recovery failure and must not claim that a peer cleared the rebase. The PR's changed abort claim flow is the direct cause, so a targeted correction to its error decision is appropriate.
**Relevant code:**
`src/doltlite_rebase.c:1057-1063`
~~~C
rc = chunkStoreLockAndRefresh(cs);
if( rc!=SQLITE_OK ) return rc;
...
rc = chunkStoreForceRefresh(cs);
if( rc!=SQLITE_OK ) goto claim_done;
rc = doltliteLoadWorkingSet(db, zBranch);
if( rc!=SQLITE_OK ) goto claim_done;
~~~
`src/doltlite_rebase.c:1404-1447`
~~~C
rc = rebaseClaimActiveEndRetry(db);
...
if( rc!=SQLITE_OK ){
...
if( !stillRebasing ){
sqlite3_result_error(context, "no rebase in progress", -1);
}else{
rebaseResultRecoveryFailure(context, rc);
}
return;
}
...
sqlite3_result_text(context, "Interactive rebase aborted", -1, SQLITE_STATIC);
~~~
`src/doltlite_rebase.c:1544-1550`
~~~C
rc = rebaseClaimActiveEndRetry(db);
if( rc==SQLITE_DONE ){ ... }
if( rc!=SQLITE_OK ) goto abort_err;
bPlanDropped = 1;
~~~
`test/doltlite_regression_test_c.c:6948-6959`
~~~C
gRegressionFaultCode = 953;
...
res = queryScalarText(db, "SELECT dolt_rebase('--abort')");
...
check("rebase_abort_recovery_failure_is_returned",
strstr(res, "ERROR: rebase recovery failed")!=0);
~~~
added 3 commits
August 6, 2026 09:40
Left over after best-effort cleanup stopped using it; fails -Werror unused-variable on assert-enabled / dead-code gates.
Ito regressions: fault 953 (DROP during claim) was reporting "no rebase in progress" or silent success because isRebasing was cleared before DROP failed. - Drop the plan table before clearing durable isRebasing so a storage fault during DROP leaves the rebase flag set and returns recovery failed. - Only report "no rebase in progress" when claim returns SQLITE_DONE (successful durable read with isRebasing clear). - After a successful claim, soft-succeed cleanup on BUSY/LOCKED or a missing temp branch (concurrent --continue), but keep hard cleanup errors as recovery failed.
Empty commit to restart CI; previous failures were Service Unavailable while resolving action download info, not product code.
|
Diff SummaryCoverage spans normal rebase completion and cancellation, reopening and persistence across connections, concurrent continue/abort ownership, branch and repository cleanup, and storage-failure recovery. The broader behavior is generally healthy, but fresh or reopened connections cannot reliably continue a valid rebase, exposing a core workflow failure. Not safe to merge yet — this PR introduces a high-severity regression in completing valid rebases after a connection change, with a related medium-severity failure for reopened rebases that can discard the expected replayed changes. An unrelated medium-severity missing-session-branch finding is a flag for later, not a driver of this verdict. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Abort rejects a missing session branch
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
| ** Drop the plan table before clearing isRebasing so a fault during DROP leaves | ||
| ** the durable flag set (fault 953 / recovery-failure regressions). Callers must | ||
| ** copy branch names out of session state before claiming. */ | ||
| static int rebaseClaimActiveEnd(sqlite3 *db){ |
There was a problem hiding this comment.
🆕 New Failure: identified in this diff run
Reopened rebase cannot continue
What failed: The reopened database showed the saved plan and temporary branch, but continue returned 'rebase failed — branch restored to pre-rebase state'. The temporary state was cleared and a later commit succeeded, yet the expected replayed feature changes were never applied.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Users who reopen an unfinished rebase cannot continue it, so the saved changes are not replayed. The branch is restored, but users must recover the rebase another way instead of completing the expected workflow.
- Steps to Reproduce:
- Create a main branch and a feature branch with committed changes, then start an interactive rebase from the feature branch onto main.
- Close the connection that started the rebase before continuing.
- Open a fresh connection and confirm that the saved plan has one row and the temporary rebase branch exists.
- Run dolt_rebase('--continue').
- Inspect the branches and make a normal commit after the command returns.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: The relevant implementation is src/doltlite_rebase.c. The PR adds rebaseClaimActiveEnd at lines 1066-1125: it refreshes and reloads the working set, drops main.dolt_rebase at lines 1095-1097, then clears and persists isRebasing at lines 1112-1119. The continue path reads the plan at lines 1553-1561, then the PR-added claim-before-replay block at lines 1603-1611 calls rebaseClaimActiveEndRetry and marks bPlanDropped before replay starts. Replay and branch finalization then depend on the in-memory plan and branch context through rebaseReplayPlanGroup and doltliteMutateRefs at lines 1613-1647. The recorded fresh-connection run proves that durable metadata was readable before continue, but this path still failed and entered abort_err at lines 1701-1735, which reports the restored-branch error after cleanup. This narrows the defect to the changed claim/replay/finalization interaction rather than missing setup: the claim must not invalidate the durable replay context, or continue must reload that context after claiming. A targeted fix should retain a valid plan/context snapshot through the claim or reopen and validate it before replay, while keeping the one-owner behavior.
- Why this is likely a bug: This is a real product failure because the public dolt_rebase('--continue') operation rejects a valid, persisted rebase after a normal close-and-reopen. The test did not inject faults, mocks, or unsupported commands: it observed plan_rows=1 and temp_ref=1 before the operation, then the application itself returned the recovery error and restored the branch. The PR explicitly changes the ownership and cleanup ordering used by continue, and those changed lines clear the durable state immediately before replay; the failure is therefore directly tied to the PR's new path. The practical fix is targeted: keep the valid durable plan and branch names available after the ownership claim, or reload them before replay and final ref cleanup, then retain the existing post-claim cleanup behavior.
Relevant code
src/doltlite_rebase.c:1066-1119
static int rebaseClaimActiveEnd(sqlite3 *db){ ... sqlite3_exec(db, "DROP TABLE IF EXISTS main.dolt_rebase", ...); ... doltliteClearSessionRebaseState(db); ... doltliteSaveWorkingSet(db);src/doltlite_rebase.c:1551-1614
rc = doltliteValidateRebasePlanTable(db, &zPlanErr); ... rc = rebaseReadPlan(db, &aPlan, &nPlan); ... rc = rebaseClaimActiveEndRetry(db); ... bPlanDropped = 1; ... rc = doltliteFlushCatalogToHash(db, &curCat);src/doltlite_rebase.c:1701-1735
abort_err: ... recoveryRc = rebaseDiscardWorkingBranch(...); ... sqlite3_result_error(context, "rebase failed — branch restored to pre-rebase state", -1);Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Reopened rebase cannot continue**
**What failed:** The reopened database showed the saved plan and temporary branch, but continue returned 'rebase failed — branch restored to pre-rebase state'. The temporary state was cleared and a later commit succeeded, yet the expected replayed feature changes were never applied.
- **Impact:** Users who reopen an unfinished rebase cannot continue it, so the saved changes are not replayed. The branch is restored, but users must recover the rebase another way instead of completing the expected workflow.
- **Steps to reproduce:**
1. Create a main branch and a feature branch with committed changes, then start an interactive rebase from the feature branch onto main.
2. Close the connection that started the rebase before continuing.
3. Open a fresh connection and confirm that the saved plan has one row and the temporary rebase branch exists.
4. Run dolt_rebase('--continue').
5. Inspect the branches and make a normal commit after the command returns.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** The relevant implementation is src/doltlite_rebase.c. The PR adds rebaseClaimActiveEnd at lines 1066-1125: it refreshes and reloads the working set, drops main.dolt_rebase at lines 1095-1097, then clears and persists isRebasing at lines 1112-1119. The continue path reads the plan at lines 1553-1561, then the PR-added claim-before-replay block at lines 1603-1611 calls rebaseClaimActiveEndRetry and marks bPlanDropped before replay starts. Replay and branch finalization then depend on the in-memory plan and branch context through rebaseReplayPlanGroup and doltliteMutateRefs at lines 1613-1647. The recorded fresh-connection run proves that durable metadata was readable before continue, but this path still failed and entered abort_err at lines 1701-1735, which reports the restored-branch error after cleanup. This narrows the defect to the changed claim/replay/finalization interaction rather than missing setup: the claim must not invalidate the durable replay context, or continue must reload that context after claiming. A targeted fix should retain a valid plan/context snapshot through the claim or reopen and validate it before replay, while keeping the one-owner behavior.
- **Why this is likely a bug:** This is a real product failure because the public dolt_rebase('--continue') operation rejects a valid, persisted rebase after a normal close-and-reopen. The test did not inject faults, mocks, or unsupported commands: it observed plan_rows=1 and temp_ref=1 before the operation, then the application itself returned the recovery error and restored the branch. The PR explicitly changes the ownership and cleanup ordering used by continue, and those changed lines clear the durable state immediately before replay; the failure is therefore directly tied to the PR's new path. The practical fix is targeted: keep the valid durable plan and branch names available after the ownership claim, or reload them before replay and final ref cleanup, then retain the existing post-claim cleanup behavior.
**Relevant code:**
`src/doltlite_rebase.c:1066-1119`
~~~c
static int rebaseClaimActiveEnd(sqlite3 *db){ ... sqlite3_exec(db, "DROP TABLE IF EXISTS main.dolt_rebase", ...); ... doltliteClearSessionRebaseState(db); ... doltliteSaveWorkingSet(db);
~~~
`src/doltlite_rebase.c:1551-1614`
~~~c
rc = doltliteValidateRebasePlanTable(db, &zPlanErr); ... rc = rebaseReadPlan(db, &aPlan, &nPlan); ... rc = rebaseClaimActiveEndRetry(db); ... bPlanDropped = 1; ... rc = doltliteFlushCatalogToHash(db, &curCat);
~~~
`src/doltlite_rebase.c:1701-1735`
~~~c
abort_err: ... recoveryRc = rebaseDiscardWorkingBranch(...); ... sqlite3_result_error(context, "rebase failed — branch restored to pre-rebase state", -1);
~~~
Claim the canonical interactive-rebase working state before either terminal operation drops the plan or opens its write path. This keeps the loser read-only and prevents stale refs from restoring the temporary branch. Require exactly one race winner, verify winner-specific data and cleanup, cover reopen/continue on ordinary branch names, and record the concurrency guarantee. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Check the race barrier pipe operations so fortified checked builds do not reject ignored I/O results. Re-read canonical rebase state under the graph lock when continue loses before claiming, mapping a concurrently removed plan to the required no-rebase result. Co-Authored-By: OpenAI Codex <noreply@openai.com>
|
Diff SummaryThe run broadly exercised rebase completion and cancellation, reopening and cleanup, concurrent operations, storage and lock failures, malformed or missing state, and recovery of the repository for subsequent work. These happy-path, edge-case, and adversarial scenarios were largely healthy, but the concurrency validation does not fully enforce completion outcomes. Merge with caution — the PR introduces a medium-severity gap in concurrent-operation validation that could allow an incomplete child process to be accepted as a successful result, weakening protection against faulty race behavior. The issue does not indicate direct data loss or corruption, but it should be addressed before relying on this test as a merge safeguard. Tests run by Ito
Tip Reply with @itoqa to send us feedback on this test run. |
| abortOut[n] = 0; | ||
| close(pipefd[0]); | ||
| } | ||
| waitpid(child, &status, 0); |
There was a problem hiding this comment.
🆕 New Failure: identified in this diff run
Race checker accepts incomplete child runs
What failed: The checker decides whether a trial passed from the two child output strings, but it does not reject a child that exited with an error or signal. A child that did not complete can therefore be counted as a valid winner or loser.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: A faulty concurrent rebase result could reach users without this test catching it. There is no evidence that this defect directly causes user data loss or corruption.
- Steps to Reproduce:
- Build and run the multiprocess rebase test.
- Arrange for the abort child to print a winner-like result and then exit with a nonzero status or because of a signal.
- Let the parent collect both output strings and finish the trial.
- Observe that the trial can still be accepted when the output strings contain exactly one winner and one loser message.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: The recorded normal run completed all 45 assertions with 12 concurrent continue-versus-abort trials, wins_continue=4, wins_abort=8, and bad=0. The separate interrupted run ended with exit 124 before child-output collection, so it is not proof of an application rebase failure; the defect is established by source inspection. In the PR-added test/multi_process_merge_rebase_test.c, the parent calls waitpid(child, &status, 0) at lines 606-606, but does not inspect status afterward. Lines 608-611 derive contWin, abortWin, contLost, and abortLost only by searching contOut and abortOut. Lines 612-613 then accept the trial when exactlyOneWin and loserOk are true, with no WIFEXITED or WEXITSTATUS condition. Therefore a child can write a valid-looking response and then terminate abnormally, while the parent still accepts the trial. This directly violates CONTRACT-20's incomplete-child oracle requirement. The targeted remediation is to validate the wait status immediately after waitpid and include successful normal child exit in the acceptance predicate, treating signaled, nonzero, or failed wait results as bad trials.
- Why this is likely a bug: This is not caused by a mock or setup failure: the PR changes the actual test oracle, and the missing status check is visible in the code path that decides whether a trial is valid. The test's stated contract requires incomplete child events to fail, yet the acceptance condition can be satisfied using output emitted before abnormal termination. A focused wait-status check is sufficient; no broad production-code change is needed.
Relevant code
test/multi_process_merge_rebase_test.c:606-613
waitpid(child, &status, 0);
contWin = strstr(contOut, "Successfully")!=0;
abortWin = strstr(abortOut, "Interactive rebase aborted")!=0;
contLost = strstr(contOut, "no rebase in progress")!=0;
abortLost = strstr(abortOut, "no rebase in progress")!=0;
exactlyOneWin = contWin ^ abortWin;
loserOk = (contWin && abortLost) || (abortWin && contLost);test/multi_process_merge_rebase_test.c:617-621
if( !exactlyOneWin || !loserOk ){
bad++;
fprintf(stderr, "FAIL trial %d cont=[%s] abort=[%s]\n",
trial, contOut, abortOut);
}Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Race checker accepts incomplete child runs**
**What failed:** The checker decides whether a trial passed from the two child output strings, but it does not reject a child that exited with an error or signal. A child that did not complete can therefore be counted as a valid winner or loser.
- **Impact:** A faulty concurrent rebase result could reach users without this test catching it. There is no evidence that this defect directly causes user data loss or corruption.
- **Steps to reproduce:**
1. Build and run the multiprocess rebase test.
2. Arrange for the abort child to print a winner-like result and then exit with a nonzero status or because of a signal.
3. Let the parent collect both output strings and finish the trial.
4. Observe that the trial can still be accepted when the output strings contain exactly one winner and one loser message.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** The recorded normal run completed all 45 assertions with 12 concurrent continue-versus-abort trials, wins_continue=4, wins_abort=8, and bad=0. The separate interrupted run ended with exit 124 before child-output collection, so it is not proof of an application rebase failure; the defect is established by source inspection. In the PR-added test/multi_process_merge_rebase_test.c, the parent calls waitpid(child, &status, 0) at lines 606-606, but does not inspect status afterward. Lines 608-611 derive contWin, abortWin, contLost, and abortLost only by searching contOut and abortOut. Lines 612-613 then accept the trial when exactlyOneWin and loserOk are true, with no WIFEXITED or WEXITSTATUS condition. Therefore a child can write a valid-looking response and then terminate abnormally, while the parent still accepts the trial. This directly violates CONTRACT-20's incomplete-child oracle requirement. The targeted remediation is to validate the wait status immediately after waitpid and include successful normal child exit in the acceptance predicate, treating signaled, nonzero, or failed wait results as bad trials.
- **Why this is likely a bug:** This is not caused by a mock or setup failure: the PR changes the actual test oracle, and the missing status check is visible in the code path that decides whether a trial is valid. The test's stated contract requires incomplete child events to fail, yet the acceptance condition can be satisfied using output emitted before abnormal termination. A focused wait-status check is sufficient; no broad production-code change is needed.
**Relevant code:**
`test/multi_process_merge_rebase_test.c:606-613`
~~~c
waitpid(child, &status, 0);
contWin = strstr(contOut, "Successfully")!=0;
abortWin = strstr(abortOut, "Interactive rebase aborted")!=0;
contLost = strstr(contOut, "no rebase in progress")!=0;
abortLost = strstr(abortOut, "no rebase in progress")!=0;
exactlyOneWin = contWin ^ abortWin;
loserOk = (contWin && abortLost) || (abortWin && contLost);
~~~
`test/multi_process_merge_rebase_test.c:617-621`
~~~c
if( !exactlyOneWin || !loserOk ){
bad++;
fprintf(stderr, "FAIL trial %d cont=[%s] abort=[%s]\n",
trial, contOut, abortOut);
}
~~~
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes concurrent
dolt_rebase('--continue')vsdolt_rebase('--abort')both failing.Before: both connections saw session
isRebasing, both ran multi-step cleanup, both could reportrebase failed/rebase recovery failed.After: one side claims the end of the rebase under the graph lock (reload durable working-set state → clear
isRebasing+ persist → dropdolt_rebase). The loser getsno rebase in progress. Cleanup after claim is idempotent if the temp working branch is already gone.Matches Dolt’s
abortRebase/validateActiveRebasespirit: once working-set rebase state is cleared, the other session should see no active rebase.Test plan
multi_process_merge_rebase_test— 44/44 (includes new Test 5: 12 concurrent continue/abort trials)test/doltlite_rebase.sh— 24/24no rebase in progress