Skip to content

Add the ability to release the locks associated with reversed Txns#9045

Merged
kring merged 116 commits into
masterfrom
kring/release-txn-locks
Apr 14, 2026
Merged

Add the ability to release the locks associated with reversed Txns#9045
kring merged 116 commits into
masterfrom
kring/release-txn-locks

Conversation

@kring

@kring kring commented Mar 2, 2026

Copy link
Copy Markdown
Member

Fixes #8552 by making ServerBasedLocks Txn-aware, so locks from reversed or canceled Txns can be safely abandoned and later reacquired for redo, without weakening existing lock guarantees.

This PR:

  • Tracks lock acquisition per Txn in the ServerBasedLocks lock cache, including upgrades of Shared -> Exclusive.
  • Adds async versions of TxnManager functions for Txn reverse and cancel that automatically perform lock abandon, and functions for Txn reinstate that automatically perform lock reacquisition.
  • Makes lock abandonment on reverse optional, but, if locks are abandoned, then they must be reacquired in order to reinstate.
  • Keeps existing synchronous TxnManager functions intact and does not change their semantics, to preserve backward compatibility.
  • Ensures even the old synchronous reinstateTxn fails with LockNotHeld when required locks were previously abandoned, so lock semantics cannot be violated. This does not contradict the point above, and is not a breaking change, because clients using the synchronous reverse/cancel functions will not abandon the locks in the first place.
  • Clears stale Txn-lock records when Txns become unreachable (canceled), preventing incorrect behavior when Txn IDs are reused.
  • Switches discard-changes lock release semantics to abandon mode so lock release does not stamp a changeset when nothing was actually pushed, avoiding unnecessary pull requirements for other users.
  • Introduces optional hub access abandon APIs with compatibility fallbacks to existing lock update calls, so older hub implementations continue to work.

Backward compatibility:

  • Existing synchronous Txn APIs remain available and behavior-compatible unless apps opt into the new async lock-aware flows.
  • New BackendHubAccess methods are optional and ServerBasedLocks knows how to use fallbacks when they're not available.
  • LockControl is not available for external implementation, so new methods there do not represent breaking changes.

Lock-correctness guarantees:

  • Locks are only abandoned after 1) unsaved changes have been abandoned, and 2) the Txn in which the locks were acquired has been reversed.
  • Reinstating a reversed Txn requires the previously-acquired locks to be held. Users can't accidentally (or purposely) reinstate changes without first accquiring the necessary locks.
  • In the (unlikely?) event lock abandonment fails while canceling a Txn, the transaction remains reinstateable rather than being irreversibly canceled. This allows the abandonment to be retried, or alternatively the client can proceed to cancel without abandoning the locks.

kring added 26 commits February 19, 2026 21:38
"Abandoning" a lock sounds more drastic than what we're actually doing, which is releasing the lock after any changes to the associated elements have been abandoned.
@kring kring requested a review from a team as a code owner March 2, 2026 10:41

@aruniverse aruniverse left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

changes look fine to me, approving. do wonder if we should call this out in nextver.md
@khanaffan @rschili @simihartstein plz review

@kring

kring commented Mar 31, 2026

Copy link
Copy Markdown
Member Author

I built Studio against this branch and ran OS+ (unmodified) in it. And much to my surprise, OS+ undo/redo operations don't call into the code I modified at all! TIL about IpcHost.ts. The reverse and reinstate functions on that class don't call the methods on IModelDb. Instead, they directly pull the _nativeDb field out of the instance and call straight into the native code. See here.

So I guess I will fix that. Looks like I will also need to add the new async functions here and to BriefcaseTxns so that they can be called from the frontend.

@mergify

mergify Bot commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

This pull request is now in conflicts. Could you fix it @kring? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

@kring

kring commented Apr 10, 2026

Copy link
Copy Markdown
Member Author

I've now tried this branch in OS+, and made some further changes based on that experience. A few notes:

  1. I previously hadn't exposed the new async reverse/reinstate methods through the frontend's BriefcaseTxns. Now I have.
  2. Previously, calls to the sync reverse/reinstate functions were going directly to the imodel-native layer, instead of calling into TxnManager. I've now changed this in this PR. However, it did require this slightly awkward structure because not all IModelDb instances have a TxnManager:
    const db = IModelDb.findByKey(key);
    if (db.isBriefcaseDb()) {
      return db.txns.reverseTxns(numOperations);
    } else {
      return db[_nativeDb].reverseTxns(numOperations);
    }

I don't think it makes any sense to call reverseTxns on something that's not a briefcase DB, and so it would be fine to throw an exception instead, but I wasn't sure. For the new async methods, where there are fewer backward compatibility concerns, I've chosen to throw.

  1. I was unwittingly passing element IDs as decimal strings (rather than hexadecimal) to BackendHubAccess methods. HubMock was ok with this, but the real Hub was not. Now fixed.
  2. I had a lot of trouble running OS+ / local Studio / custom itwinjs-core in the VS Code debugger. The Extension Host process hosting the javascript debugger kept exceeding the heap space limit (4GB) and crashing. After trying many things to fix this, none of which worked, I eventually learned I could use the Chrome DevTools instead, which was much more reliable. I have instructions for this whole process written down, which I should share somewhere.
  3. OS+'s undo/redo appears to be handled entirely within itwinjs-core, specifically core/frontend/src/tools/ToolAdmin.ts. So now this PR modifies that code to use the new async methods, which automatically abandon and reacquire locks. OS+ should get this new behavior without any further changes.
  4. The lock abandonment seems to be working as expected. I tried opening the same iModel in OS+ on two different computers. When I make a change on one, the object turns red on the other. When I undo that change, the red highlight on the other disappears.
  5. If I make a change on one computer, undo it, make a conflicting change on the other computer, and then attempt to Redo on the first, OS+ silently does nothing. I see the following in the log:

2026-04-10 11:29:06.724 [error] |iTwinStudio.Backend| ConflictWithAnotherUser: Lock(s) is owned by another Briefcase. Conflicting locks:

  1. Object id: 0x200000004ef, lock level: exclusive, briefcase ids: 3

Response from requested url: undefined, status code: undefined

Could be some room for improvement here, but it doesn't seem too bad.

  1. I noticed that OS+ acquires a bunch of shared locks at startup. This happens in apps/booster/src/backend/Site.ts startEditScope method. In this PR, those shared locks get associated with the initial Txn. Which means that if we edit something in that Txn, and then undo it, those locks are abandoned as well. If we then redo that operation, the locks come back, which is fine. But if instead of redoing we make some other edit, those shared locks will never be reacquired. I don't know exactly why these shared locks are needed, but I'm guessing this is a problem. I'd appreciate any thoughts on this.

@kring

kring commented Apr 13, 2026

Copy link
Copy Markdown
Member Author

@kabentley you have a couple of unresolved comments above, which I think are now the only thing stopping this PR from being auto-merged. Mind taking a look and seeing if you have any concerns? Thanks!

@pmconne

pmconne commented Apr 13, 2026

Copy link
Copy Markdown
Member

not all IModelDb instances have a TxnManager

Use BriefcaseDb.findByKey instead. EDIT: that function uses as. It should be changed to throw if an instanceof BriefcaseDb check fails.

@kring

kring commented Apr 13, 2026

Copy link
Copy Markdown
Member Author

Thanks @pmconne.

Use BriefcaseDb.findByKey instead. EDIT: that function uses as. It should be changed to throw if an instanceof BriefcaseDb check fails.

Looks like BriefcaseDb.tryFindByKey should work well. But are you suggesting I do this in all methods, or only in the new async ones?

@pmconne

pmconne commented Apr 13, 2026

Copy link
Copy Markdown
Member

Looks like BriefcaseDb.tryFindByKey should work well. But are you suggesting I do this in all methods, or only in the new async ones?

I spelunked into the rabbit hole and found that BriefcaseDb.findByKey calls super.findByKey which calls this.tryFindByKey in which case this is the class BriefcaseDb which will invoke BriefcaseDb's override of tryFindByKey which returns undefined if the iModel is not a BriefcaseDb. That's not at all clear from even a moderately attentive reading of the code (see here), but it means that wherever you require a BriefcaseDb (e.g., when reversing txns, which only makes sense for briefcases), you should use BriefcaseDb.findByKey and not have to worry about handling the "not a briefcase" path, nor the undefined path - it'll throw, as desired, if no briefcase is associated with the specified key.

@kring

kring commented Apr 14, 2026

Copy link
Copy Markdown
Member Author

Ok thanks @pmconne, I've changed IpcHost.ts to use BriefcaseDb.findByKey in all the reverse and reinstate methods, including both the existing sync and the new async ones.

@kring kring merged commit 96b4153 into master Apr 14, 2026
18 checks passed
@kring kring deleted the kring/release-txn-locks branch April 14, 2026 01:59
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.

Automatically release locks when undoing Txns