Skip to content

Commit c26467b

Browse files
authored
fix(ci): recover from a stale remote lane on --keep-lane (#10400)
## Summary Surfaced by #10397's `bit_pr` failure. When `--keep-lane` (from #10388) found the lane on the remote, it called `switchToLane` and assumed that landed us on the lane. If the stored lane was stale relative to the current PR — typically referencing a `ModelComponent` the PR has since removed/renamed, surfaced as `unable to merge lane …, the component … was not found` — the switch failed (`switchToLane` logs-and-swallows the underlying error), the post-switch guard then threw, and every subsequent `bit ci pr --keep-lane` on that branch wedged the same way until the lane was manually deleted on the remote. ## Fix When we observe that we did not land on the requested lane after switching, delete the stale remote lane and create a fresh one (same name) — the same shape the `else` branch already takes when the lane doesn't exist on the remote. Lane history for the contested run is lost, but the next run preserves history again and CI is no longer blocked.
1 parent 045a559 commit c26467b

1 file changed

Lines changed: 119 additions & 28 deletions

File tree

scopes/git/ci/ci.main.runtime.ts

Lines changed: 119 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -329,7 +329,13 @@ export class CiMain {
329329
return { status };
330330
}
331331

332-
private async switchToLane(laneName: string, options: SwitchLaneOptions = {}) {
332+
/**
333+
* Returns the caught Error on failure, or undefined on success (including the "already checked
334+
* out" no-op case). Callers that need to react to a specific failure mode (e.g. stale lane) can
335+
* inspect the returned error; existing callers ignore it and rely on a follow-up
336+
* `getCurrentLane()` check.
337+
*/
338+
private async switchToLane(laneName: string, options: SwitchLaneOptions = {}): Promise<Error | undefined> {
333339
this.logger.console(chalk.blue(`Switching to ${laneName}`));
334340
try {
335341
await this.lanes.switchLanes(laneName, {
@@ -339,12 +345,14 @@ export class CiMain {
339345
...options,
340346
});
341347
} catch (e: any) {
342-
if (e.toString().includes('already checked out')) {
348+
if (e?.toString().includes('already checked out')) {
343349
this.logger.console(chalk.yellow(`Lane ${laneName} already checked out, skipping checkout`));
344-
return true;
350+
return undefined;
345351
}
346-
this.logger.console(chalk.red(`Failed switching to ${laneName}: ${e.toString()}`));
352+
this.logger.console(chalk.red(`Failed switching to ${laneName}: ${e?.toString() ?? e}`));
353+
return e;
347354
}
355+
return undefined;
348356
}
349357

350358
/**
@@ -610,35 +618,118 @@ export class CiMain {
610618
// lane-history feature on Bit Cloud all survive across subsequent commits to the same PR.
611619
// switchToLane fetches the latest lane head from remote.
612620
this.logger.console(chalk.blue(`Lane ${laneId.toString()} exists on remote, reusing it`));
613-
await this.switchToLane(laneId.toString());
614-
// Verify the switch actually landed us on the lane before doing any lane work.
615-
// switchToLane logs-and-swallows switch failures, so without this guard a failed switch
616-
// would let syncConfigFromMain and snap run against the wrong lane.
621+
const switchErr = await this.switchToLane(laneId.toString());
622+
// switchToLane returns the caught error (undefined on success). Combine with a
623+
// current-lane-state probe — comparing BOTH name AND scope, so a same-named lane in a
624+
// different scope can't masquerade as a successful switch.
617625
const switchedLane = await this.lanes.getCurrentLane();
618-
if (switchedLane?.name !== laneId.name) {
619-
throw new Error(
620-
`Expected to be on lane ${laneId.name} after switching, but current lane is ${switchedLane?.name ?? 'main'}`
621-
);
622-
}
623-
// Sync config-only changes from main onto the lane, so config that was tagged into objects
624-
// on main since the lane forked (e.g. `bit deps set` / `bit env set` from another PR, not
625-
// visible via the workspace's git checkout) is reflected on the lane. Source files are
626-
// git's job — see syncConfigFromMain.
627-
//
628-
// BUT only when the PR branch is actually up to date with the default branch. If the PR is
629-
// behind (hasn't pulled main's latest), its git checkout still reflects the older fork
630-
// point, so pulling main's newer config onto the lane would desync the lane from the
631-
// source. The author merges the default branch into their PR in git; the next `bit ci pr`
632-
// then propagates it here.
633-
if (await this.isBranchBehindDefaultBranch()) {
626+
const landedOnLane = switchedLane?.name === laneId.name && switchedLane?.scope === laneId.scope;
627+
if (landedOnLane) {
628+
// Sync config-only changes from main onto the lane, so config that was tagged into
629+
// objects on main since the lane forked (e.g. `bit deps set` / `bit env set` from
630+
// another PR, not visible via the workspace's git checkout) is reflected on the lane.
631+
// Source files are git's job — see syncConfigFromMain.
632+
//
633+
// BUT only when the PR branch is actually up to date with the default branch. If the PR
634+
// is behind (hasn't pulled main's latest), its git checkout still reflects the older
635+
// fork point, so pulling main's newer config onto the lane would desync the lane from
636+
// the source. The author merges the default branch into their PR in git; the next
637+
// `bit ci pr` then propagates it here.
638+
if (await this.isBranchBehindDefaultBranch()) {
639+
this.logger.console(
640+
chalk.yellow(
641+
`PR branch is behind the default branch — skipping config sync from main. ` +
642+
`Merge or rebase the default branch into your PR to pick up main's latest config.`
643+
)
644+
);
645+
} else {
646+
await this.syncConfigFromMain(laneId);
647+
}
648+
} else {
649+
// Switch failed even though the remote lane exists. The destructive recovery below
650+
// (delete the remote lane + recreate fresh) is safe only when the failure is the
651+
// specific "stale lane" pattern — the lane references a ModelComponent the PR has
652+
// since removed/renamed (`unable to merge lane …, the component … was not found`).
653+
// For any other failure (transient network blip during fetch, auth error, lane locked
654+
// by Cloud UI, etc.) destroying lane history would be the wrong response, so we
655+
// rethrow and let the caller report the real cause.
656+
const errMsg = switchErr?.toString() ?? '';
657+
const isStaleLane = errMsg.includes('unable to merge lane');
658+
if (!isStaleLane) {
659+
throw new Error(
660+
`Failed to switch to remote lane ${laneId.toString()}: ${errMsg || '(no error captured)'}. ` +
661+
`Refusing destructive recovery for this failure class — the error doesn't match the ` +
662+
`stale-lane marker, so deleting the lane could destroy real history. Investigate or retry.`
663+
);
664+
}
634665
this.logger.console(
635666
chalk.yellow(
636-
`PR branch is behind the default branch — skipping config sync from main. ` +
637-
`Merge or rebase the default branch into your PR to pick up main's latest config.`
667+
`Stale remote lane ${laneId.toString()} — switching failed. ` +
668+
`Deleting it and creating a fresh lane to recover.`
638669
)
639670
);
640-
} else {
641-
await this.syncConfigFromMain(laneId);
671+
// Re-check the remote lane's hash immediately before deleting. The central-hub delete
672+
// API is name-based — there's no compare-and-swap — so two CI jobs racing the same
673+
// recovery could otherwise have job B delete job A's freshly-recreated lane. By
674+
// re-fetching here we shrink the TOCTOU window to milliseconds: if A's recreate landed
675+
// before our re-fetch, the hash changed and we skip the delete entirely. The downstream
676+
// export then hits the lane-hash mismatch and lands in `exportWithAdoptOnConflict`,
677+
// which rebases our snaps onto the winner's lane — no destroyed history.
678+
const staleHash = existingLanes[0]?.hash;
679+
const recheck = await this.lanes.getLanes({ remote: laneId.scope, name: laneId.name }).catch(() => []);
680+
const currentRemoteHash = recheck[0]?.hash;
681+
const remoteChanged = staleHash && currentRemoteHash && currentRemoteHash !== staleHash;
682+
if (remoteChanged) {
683+
this.logger.console(
684+
chalk.blue(
685+
`Remote lane ${laneId.toString()} changed since we first checked (hash ` +
686+
`${staleHash.slice(0, 9)}${currentRemoteHash.slice(0, 9)}) — another concurrent ` +
687+
`recovery already recreated it. Skipping the delete; export will adopt-on-conflict.`
688+
)
689+
);
690+
} else {
691+
await this.lanes.removeLanes([laneId.toString()], { remote: true, force: true }).catch((e) => {
692+
const msg = e?.toString() ?? '';
693+
// Tolerate the race where another concurrent recovery deleted the lane first — the
694+
// desired post-condition (lane gone from remote) is already met.
695+
if (msg.includes('was not found') || msg.includes('not found')) {
696+
this.logger.console(chalk.blue(`Remote lane ${laneId.toString()} was already gone — proceeding`));
697+
return;
698+
}
699+
throw new Error(`Failed to delete stale remote lane ${laneId.toString()}: ${msg || e}`);
700+
});
701+
}
702+
// switchToLane fetched the remote lane and persisted it into the local scope's lane
703+
// index (via `importLaneObject` → `legacyScope.lanes.saveLane`) BEFORE the underlying
704+
// merge failed. Without dropping that local copy here, the upcoming `createLane` would
705+
// hit the "lane … already exists" guard in create-lane.ts. Same trash-the-local-object
706+
// pattern as `rebaseOntoRemoteLane`.
707+
const legacyScope = this.workspace.scope.legacyScope;
708+
const localLane = await legacyScope.loadLane(laneId);
709+
if (localLane) {
710+
await legacyScope.objects.moveObjectsToTrash([localLane.hash()]);
711+
}
712+
// Reset the workspace's current-lane pointer to main before createLane, so the new lane
713+
// is forked from main with an empty component list. `createLane` populates new lanes
714+
// from `consumer.getCurrentLaneObject()` regardless of `forkLaneNewScope` (which only
715+
// suppresses the cross-scope guard) — if `originalLane` is non-default (a developer
716+
// running `bit ci pr` from a lane), without this reset the "fresh" lane would silently
717+
// inherit `originalLane`'s components. Check the return value: a silent failure here
718+
// would defeat the whole point of the reset.
719+
const resetErr = await this.switchToLane('main');
720+
const afterReset = await this.lanes.getCurrentLane();
721+
if (resetErr || afterReset) {
722+
throw new Error(
723+
`Failed to reset to main before recreating ${laneId.toString()}: ` +
724+
`${resetErr?.toString() ?? `(still on lane "${afterReset?.name}")`}. ` +
725+
`Aborting to avoid silently forking the recreated lane from the wrong source.`
726+
);
727+
}
728+
const createLaneResult = await this.lanes.createLane(laneId.name, {
729+
scope: laneId.scope,
730+
forkLaneNewScope: true,
731+
});
732+
this.logger.console(chalk.blue(`Recreated lane ${laneId.toString()} (hash: ${createLaneResult.hash})`));
642733
}
643734
} else {
644735
this.logger.console(chalk.blue(`Creating lane ${laneId.toString()}`));

0 commit comments

Comments
 (0)