Re: [PATCH 2/6] object-file: propagate files transaction errors

[Justin Tobler <jltobler@gmail.com>]

On 26/06/24 01:26PM, Patrick Steinhardt wrote:
> On Tue, Jun 23, 2026 at 11:19:16PM -0500, Justin Tobler wrote:
> > The "files" transaction backend may encounter errors related to managing
> > the temporary directory used to stage objects, but silently ignores
> > these errors. Instead return errors encountered in the
> > `odb_transaction_files_{prepare,begin,commit}()` interfaces to allow
> > callers to handle as needed.
> 
> Missing a then? "to handle as needed" -> "to handle them as needed"

Will fix.

[snip]
> > diff --git a/object-file.c b/object-file.c
> > index a3eb8d71dd..18c2df75fb 100644
> > --- a/object-file.c
> > +++ b/object-file.c
> > @@ -499,7 +499,7 @@ struct odb_transaction_files {
> >  	struct transaction_packfile packfile;
> >  };
> >  
> > -static void odb_transaction_files_prepare(struct odb_transaction *base)
> > +static int odb_transaction_files_prepare(struct odb_transaction *base)
> >  {
> >  	struct odb_transaction_files *transaction =
> >  		container_of_or_null(base, struct odb_transaction_files, base);
> 
> By the way, is there any reason why those functions are still hosted in
> "object-file.c" instead of in "odb/source-files.c"? I should probably
> know, but I forgot.

There are currently a couple spots in the "files" object write path in
"object-file.c" that still reach into some of these transaction function
that are not part of the generic ODB transaction interface. I'm hoping
in a future followup series to detangle this a bit and be able to get
all the "files" ODB transaction related code moved into
"odb/source-files.c".

> > @@ -511,11 +511,15 @@ static void odb_transaction_files_prepare(struct odb_transaction *base)
> >  	 * added at the time they call odb_transaction_files_begin.
> >  	 */
> >  	if (!transaction || transaction->objdir)
> > -		return;
> > +		return 0;
> >  
> >  	transaction->objdir = tmp_objdir_create(base->source->odb->repo, "bulk-fsync");
> > -	if (transaction->objdir)
> > -		tmp_objdir_replace_primary_odb(transaction->objdir, 0);
> > +	if (!transaction->objdir)
> > +		return -1;
> 
> Huh. So previously we just didn't handle this error at all and just
> continued to tag along? Did that result in anything sensible or was this
> just YOLOing it?

Good question. Previously if there was an error, we wouldn't end up
creating any tmpdir and would instead continue to use the primary ODB to
write objects in. This change would make it a hard error if we fail to
create the temp dir. This matches the behavior that git-receive-pack(1)
expects, but I didn't consider that the existing callers could
transparently handle there being no temp dir.

I suspect we may want existing ODB transaction users to continue being
resilient in the same manner. In the next version, I'll maintain the
same behavior.

> > @@ -542,13 +546,13 @@ static void fsync_loose_object_transaction(struct odb_transaction *base,
> >  /*
> >   * Cleanup after batch-mode fsync_object_files.
> >   */
> > -static void flush_loose_object_transaction(struct odb_transaction_files *transaction)
> > +static int flush_loose_object_transaction(struct odb_transaction_files *transaction)
> 
> Feels like this function should've been renamed in the preceding commit,
> as well.

Ya I agree, will address in the next version.

> >  {
> >  	struct strbuf temp_path = STRBUF_INIT;
> >  	struct tempfile *temp;
> >  
> >  	if (!transaction->objdir)
> > -		return;
> > +		return 0;
> >  
> >  	/*
> >  	 * Issue a full hardware flush against a temporary file to ensure
> > @@ -570,8 +574,12 @@ static void flush_loose_object_transaction(struct odb_transaction_files *transac
> 
> There is a call to `xmks_tempfile()` hidden that can fail, but that
> failure is already handled in that function itself by dying.
> 
> >  	 * Make the object files visible in the primary ODB after their data is
> >  	 * fully durable.
> >  	 */
> > -	tmp_objdir_migrate(transaction->objdir);
> > +	if (tmp_objdir_migrate(transaction->objdir))
> > +		return -1;
> 
> Feels like another case of YOLOing it. The migration could have failed,
> but we just ignored that failure and never told the user about it. The
> result may be silent corruption, I assume?

Ya in this case, if the migration failed, we would just continue on
which would likely result in silent corruption.

> 
> > @@ -1670,27 +1678,34 @@ int read_loose_object(struct repository *repo,
> >  	return ret;
> >  }
> >  
> > -static void odb_transaction_files_commit(struct odb_transaction *base)
> > +static int odb_transaction_files_commit(struct odb_transaction *base)
> >  {
> >  	struct odb_transaction_files *transaction =
> >  		container_of(base, struct odb_transaction_files, base);
> >  
> > -	flush_loose_object_transaction(transaction);
> > +	if (flush_loose_object_transaction(transaction))
> > +		return -1;
> >  	flush_packfile_transaction(transaction);
> > +
> > +	return 0;
> >  }
> >  
> > -struct odb_transaction *odb_transaction_files_begin(struct odb_source *source)
> > +int odb_transaction_files_begin(struct odb_source *source,
> > +				struct odb_transaction **out)
> >  {
> >  	struct odb_transaction_files *transaction;
> >  	struct object_database *odb = source->odb;
> >  
> > -	if (odb->transaction)
> > -		return NULL;
> > +	if (odb->transaction) {
> > +		*out = NULL;
> > +		return 0;
> > +	}
> >  
> >  	transaction = xcalloc(1, sizeof(*transaction));
> >  	transaction->base.source = source;
> >  	transaction->base.commit = odb_transaction_files_commit;
> >  	transaction->base.write_object_stream = odb_transaction_files_write_object_stream;
> > +	*out = &transaction->base;
> >  
> > -	return &transaction->base;
> > +	return 0;
> >  }
> 
> It's still somewhat fishy that we have this ODB-level transaction, but
> that's a preexisting issue and thus outside the scope of this patch
> series. Ideally though, it would be possible for there to be multiple
> transactions, and it would be the caller's responsibility for juggling
> these transactions. Just as it happens with reference transactions.

I completely agree. One of the current problems preventing this is that
only a single instance of tmp_objdir is allowed and stored globally.
This is done to keep atexit() cleanup simple.

My plan is to eventually convert all existing tmp_objdir callsites to
use ODB transactions which should hopefully allow us to remove the need
for a separate tmp_objdir system. At that point, we can also work to
change how temp dir cleanup is handled at exit.

Another problem is that there are also a couple of ODB transaction
callsites that require to know if there is already a pending transaction
for the repository and the transaction has not been wired down to these
callsites. My hope is that this can be addressed though as we expand ODB
transaction usage for object writes.

> > diff --git a/odb/transaction.h b/odb/transaction.h
> > index 854fda06f5..f4c1ebfaaa 100644
> > --- a/odb/transaction.h
> > +++ b/odb/transaction.h
> > @@ -17,7 +17,7 @@ struct odb_transaction {
> >  	struct odb_source *source;
> >  
> >  	/* The ODB source specific callback invoked to commit a transaction. */
> > -	void (*commit)(struct odb_transaction *transaction);
> > +	int (*commit)(struct odb_transaction *transaction);
> 
> We might want to document the returned error code here.

Will do in the next version.

Thanks,
-Justin