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.

Release note

Important

dgraph.Client.Mutate now persists writes that omit CommitNow. The signature is unchanged. A mutation sent without CommitNow previously returned err == nil and a non-nil response while persisting nothing and leaving the transaction open on the server until Dgraph timed it out; it is now committed before Mutate returns. Callers who already set CommitNow are unaffected. No action is required, but a caller who was relying on the silent drop as a dry run will now see the write land.

Same-signature semantic changes do not show up in a diff review of the call sites, so this is stated here for the notes rather than only in the godoc on Mutate and in docs/datasources/dgraph/page.md. There is no CHANGELOG in the repo — release notes are compiled from merged PRs — so the PR body is the only place this can be picked up from.

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.

[Umang01-hash]

Verified against a real dgraph (v21.03): pre-fix, a Mutate without CommitNow returns no error but the write is gone on read-back ({"q":[]}); with this PR it persists. Cross-checked every dgo/v210 claim in the comments against the pinned source (finished-on-CommitNow, Commit->ErrFinished, Discard no-op-when-finished) — all accurate, and the if mutation.CommitNow guard is necessary. Matches the migration path's txn pattern, no API change, tests fail on revert, lint/race clean. Two minor notes below.

[Umang01-hash]

Re-verified at the new head: both notes resolved (dgo line numbers replaced with method-name refs, all re-checked accurate vs dgo v210; behavior change documented in page.md). Delta is comment-only, so the earlier real-dgraph E2E still holds. Re-ran the dgraph module locally — gofmt clean, vet/build ok, go test -race passes at 71.7%, golangci-lint new-issues 0, interfaces.go unchanged (no API break). Good to merge.

[akshat-kumar-singhal]

Approve with nits. Checked against dgo at the pinned version: with CommitNow the txn is finished so Commit is skipped (no ErrFinished); without it commitOrAbort marks finished before the RPC so the deferred Discard is a no-op even on commit failure; commit conflicts (ErrAborted) now reach the caller instead of being dropped; migrations use NewTxn+Commit directly and are unaffected. Reverting dgraph.go fails Test_Mutate_TransactionHandling and Test_Mutate_DiscardSurvivesCallerCancellation.

Nit (arguably should-fix) — dgraph.go:265: context.WithoutCancel(ctx) also drops the deadline. Unreachable today, but the defer exists for a future dgo that doesn't finish the txn — then the discard RPC would have no timeout. Suggest context.WithTimeout(context.WithoutCancel(ctx), <few seconds>). (Relatedly, the "canceled request logged discard failed" scenario in 8503937 can only happen under the mock.)

Nit / design — dgraph.go:284: every non-CommitNow mutation now costs a second RPC. Same semantics in one RPC: send a shallow copy of the mutation with CommitNow = true (caller's proto untouched); the defer and branch go away.

Nit — docs/datasources/dgraph/page.md:117: "Optional … Mutate commits either way" invites dropping CommitNow, which costs the extra RPC. Suggest "Recommended: commits within the mutation RPC; without it Mutate issues a separate Commit."

Nit: the container.Dgraph interface doc now promises a commit for every implementation, but only GoFr's client enforces it. And 3cb0903's message says migrations call Mutate without CommitNow; on development they use NewTxn+Mutate+Commit.

Pre-existing (follow-up) — dgraph_test.go:24: setupDB runs defer ctrl.Finish() inside the helper, so missing gomock expectations are never reported in this package — the new Discard expectations in Test_Mutate_Success/Test_Mutate_Error pass with the fix reverted. The hand-written recordingTxn is the right workaround.

[Umang01-hash]

Deep-reviewed and independently verified at head a33664ea. Correct root-cause fix — Client.Mutate opened a txn and never resolved it, so a mutation without CommitNow staged the write into an abandoned txn (persisted nothing, err==nil, non-nil resp, txn left open). mutateInTxn now commits when CommitNow is unset, skips the redundant commit when it's set, and defers a context.WithoutCancel Discard so a canceled caller ctx can't make a committed write log a discard failure.

Verified:

• dgo v210 semantics in source: Mutate copies CommitNow (txn.go:157), Do finishes only when set (206), Commit-on-finished → ErrFinished (239), Discard-on-finished is a no-op (289). Every claim holds.
• Fail-on-revert: reverting mutateInTxn fails all 5 subtests + the cancellation test.
• Real Dgraph v21.03.0 E2E: no-CommitNow write persists 0 on development, 1 with the fix; CommitNow path unchanged.
• gofmt/vet/build clean, -race clean, golangci-lint 0 issues, coverage up.
• datasources.go is a godoc-only change — no signature change, not breaking. Migration uses the manual NewTxn path (bypasses Client.Mutate), so no double-commit and no in-repo caller relied on the silent drop.

Tests are behavior-asserting (recording fake), and the gomock ctrl.Finish() early-verify gotcha is honestly documented as a follow-up. LGTM.