fix(dgraph): commit the transaction Mutate opens

[aryanmehrotra]

Fixes #4157

Description

Client.Mutate opened a transaction, mutated, and returned — it never called Commit or Discard. Whether the write landed was decided by a field the caller set on the *api.Mutation.

dgo copies CommitNow from the caller's mutation into the request (dgo v210 txn.go:157) and marks the transaction finished only when it is set (txn.go:206). A caller who left it unset had the write staged into a transaction that was then abandoned to the garbage collector: nothing persisted, err == nil, and a non-nil response. The transaction also stayed open on the server until Dgraph timed it out.

GoFr's own migration code was exactly such a caller (pkg/gofr/migration/dgraph.go on development, no CommitNow), which is one of the three reasons Dgraph migrations never recorded anything — see #3186.

Changes

Mutate now resolves the transaction it opens, in a small mutateInTxn helper so the logging/metrics wrapper is untouched:

• commits before returning when the caller did not set CommitNow;
• leaves a CommitNow mutation alone — dgo has already finished that transaction, and Commit on a finished one returns ErrFinished (txn.go:239);
• defers Discard, which is a no-op once the transaction is finished (txn.go:289), so it only releases the paths that did not reach a commit;
• logs a failing Discard rather than turning a committed write into an error.

Behaviour for callers who already set CommitNow — including the documented example — is unchanged.

Testing

Unit. New table test Test_Mutate_TransactionHandling, five cases: commits without CommitNow, does not commit again with it, commit failure returned, mutate failure returned without committing, discard failure does not fail a committed write.

Test_Mutate_DiscardSurvivesCallerCancellation covers the deferred discard separately: it calls Mutate with an already-canceled context and asserts the context the discard receives is not done. It fails with context canceled if the discard takes the caller's context, which is what it did before review.

It asserts on a recording fake rather than gomock expectations, on purpose. setupDB calls ctrl.Finish() when it returns, which marks the controller finished before the test body starts, so the t.Cleanup verification gomock.NewController installs short-circuits (mock v0.6.0 controller.go:268) and an unmet expectation is never reported. Counting calls asserts on what happened instead. Flagged as a follow-up below rather than fixed here — turning that verification back on surfaces over-specified expectations across most of the package's existing tests, which is a much larger diff than this fix.

The tests fail without the fix. Reverting mutateInTxn to development's body fails all five subtests:

--- FAIL: Test_Mutate_TransactionHandling/commits_when_CommitNow_is_not_set          Commit calls
--- FAIL: Test_Mutate_TransactionHandling/does_not_commit_again_when_CommitNow_is_set Discard calls
--- FAIL: Test_Mutate_TransactionHandling/commit_failure_is_returned
--- FAIL: Test_Mutate_TransactionHandling/mutate_failure_is_returned_without_committing Discard calls
--- FAIL: Test_Mutate_TransactionHandling/discard_failure_does_not_fail_a_committed_write Commit calls

End to end, against a real Dgraph v21.03.0. Two mutations through Client, one without CommitNow and one with, then counted back with a query. drop_all between runs, 3 runs:

	development	this PR
persisted without CommitNow	0	1
persisted with CommitNow	1	1

Both calls returned err=<nil> and a non-nil response on development — the write simply was not there.

The e2e check needed a live Dgraph, so it is not committed; the two existing Test_Mutate_* tests gained the Discard expectation the new code path requires.

gate	result
gofmt -l	clean
go vet ./...	clean
go test ./... -count=1	pass
go test ./... -race	pass
coverage	67.4% → 71.7%
golangci-lint run ./...	0 issues (absolute, not just --new-from-rev)

Why this is a fix and not a behaviour change

The transaction Mutate opens is never handed to the caller. Mutate returns (any, error) carrying the *api.Response — there is no path by which a caller who omitted CommitNow could reach that transaction and commit it afterwards. Abandoning the write was the only reachable outcome, not one of two defensible ones, which is what makes committing the only sensible semantics rather than a judgement call between two.

Manual transaction control is a separate, untouched path: Client.NewTxn() and Client.NewReadOnlyTxn() hand back a transaction the caller drives themselves. Nothing here changes it — mutateInTxn opens and resolves its own.

Breaking Changes

None. No exported signature changes. A caller who omitted CommitNow was losing the write silently; now it is written. A caller who set it sees no change.

Follow-up, not in this PR

setupDB in pkg/gofr/datasource/dgraph/dgraph_test.go runs ctrl.Finish() before the test body, so gomock never reports a missing call for any test in the package. Removing it makes verification live and immediately fails several existing tests that declare Log/Debugf expectations which never fire. Worth its own change.

Checklist

• I have formatted my code using gofmt.
• All new code is covered by unit tests.
• This PR does not decrease the overall code coverage.
• I have reviewed the code comments and documentation for clarity.

[PiyushSingh-ZS]

Read through the fix and the tests. The bug is real and this is the right fix — a silent write loss with err == nil and a non-nil response is about the worst failure shape available, so thanks for chasing it down.

Verified

• The dgo mechanics check out: CommitNow is copied from the caller's mutation into the request and the transaction is only marked finished when it is set. A caller who omitted it had the write staged into a transaction that was then abandoned.
• The decisive point, which I think is worth making explicit in the PR description, is that Client.Mutate never hands the caller a transaction handle. So a caller who omitted CommitNow had no way to commit later even in principle — abandoning the write was the only reachable outcome, not one of two valid behaviors. That makes committing the only defensible semantics rather than a judgment call, and it is the strongest argument that this is a fix rather than a behavior change.
• mutateInTxn handles the four paths correctly: commit when CommitNow is unset, skip it when set (dgo has already finished the transaction and Commit would return ErrFinished), defer Discard as a no-op once finished, and a failing Discard logged rather than allowed to turn a committed write into an error.

The choice to assert on a recording fake rather than gomock expectations, because setupDB calls ctrl.Finish() before the test body runs and short-circuits the t.Cleanup verification, is a good catch — and flagging the broader fix as a follow-up rather than dragging a package-wide test refactor into a data-loss fix is the right scoping call.

Two small things

1. The docs edit slightly overstates it.

CommitNow: true,   // Optional: Mutate commits the write either way

It is not quite optional in the sense a reader will take it. With CommitNow: true the commit happens inside the mutation RPC; without it there is now a second Commit round trip. Both persist, but they are not equivalent, and someone reading "optional" may drop it from a hot path and pick up an extra RPC per mutation.

Maybe: // Optional: commits within the mutation RPC. Mutate commits either way.

2. Discard on a canceled context will log noise.

defer func() {
	if err := txn.Discard(ctx); err != nil {
		d.logger.Error("dgraph mutation transaction discard failed: ", err)
	}
}()

If the caller's context is canceled between the Commit returning and the deferred Discard, that logs an error for a write that succeeded. Cosmetic, since Discard is a no-op post-commit, but context.WithoutCancel(ctx) for the discard would keep the log clean.

Ordering

Worth noting for whoever merges: #4168 proposes compiling the Dgraph migrator out behind gofr_nodgraph, and touches the same call path. This one should land first.

[aryanmehrotra]

Both fixed at e25d67388.

Discard no longer inherits the caller's cancellation. It takes context.WithoutCancel(ctx) now. You're right that it's cosmetic in effect, but the shape of the noise is bad: an ERROR line reading dgraph mutation transaction discard failed: context canceled describes a write that was persisted, which is exactly the log that sends someone hunting a data-loss bug that isn't there. The discard only ever aborts a transaction already being abandoned, so dropping the deadline there costs nothing.

Test_Mutate_DiscardSurvivesCallerCancellation pins it — it fails with context canceled on the old code and passes on the new.

Docs comment reworded to your suggestion:

CommitNow: true,  // Optional: commits within the mutation RPC. Mutate commits either way.

Agreed the old wording read as "drop it", which would quietly add a Commit round trip per mutation on a hot path.

On Client.Mutate never handing back a transaction handle — agreed that's the strongest form of the argument, and I'll put it in the PR description: it makes committing the only reachable semantics rather than one of two defensible ones.

Ordering noted — this should land before #4168.