Copilot commented on code in PR #13225:
URL: https://github.com/apache/gravitino/pull/13225#discussion_r4026153167
##########
api/src/main/java/org/apache/gravitino/model/ModelCatalog.java:
##########
@@ -118,7 +129,23 @@ default Model registerModel(
throws NoSuchSchemaException, ModelAlreadyExistsException,
ModelVersionAliasesAlreadyExistException {
Model model = registerModel(ident, comment, properties);
- linkModelVersion(ident, uris, aliases, comment, properties);
+ try {
+ linkModelVersion(ident, uris, aliases, comment, properties);
+ } catch (RuntimeException e) {
+ // Best-effort compensation: drop the just-registered model so a failed
registration does
+ // not leave an orphan model with zero versions; propagate the original
failure. Only roll
+ // back a model that still has no versions, since deleteModel cascades
to all versions and
+ // another actor may have linked one concurrently.
+ try {
+ if (listModelVersions(ident).length == 0) {
+ deleteModel(ident);
Review Comment:
The emptiness check and the cascading `deleteModel` are separate operations.
If another actor links a version after `listModelVersions` returns zero but
before `deleteModel` runs, this rollback still deletes that version, so the
stated safety condition is not actually enforced and can lose another actor's
data. Please use an atomic delete-if-empty/version-checked operation (or
otherwise coordinate the check with deletion) rather than relying on this
TOCTOU check.
This issue also appears on line 140 of the same file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
