fix(session): keep Spanner ABORTED retryable through exchange conflicts - #795
Open
IAM-marco wants to merge 9 commits into
Open
fix(session): keep Spanner ABORTED retryable through exchange conflicts#795IAM-marco wants to merge 9 commits into
IAM-marco wants to merge 9 commits into
Conversation
exchangeConflict wrapped the conflict sentinel with %w but formatted the underlying cause with %v, which flattens it to a string and drops it from the error chain. Spanner's ReadWriteTransaction decides whether to retry by looking for a gRPC status in the error the callback returns (errors.As for *spanner.Error, else status.FromError). With the cause flattened it finds neither and returns immediately, so a retryable ABORTED became a user-facing conflict instead. That is the CI flakiness reported in #604: session exchange runs inside a ReadWriteTransaction, and under contention the emulator aborts it. Use %w for both operands so the conflict code stays matchable for the service and API layers while the status stays reachable for the retry. Add a regression test covering every store call that funnels through exchangeConflict, and a matching guard one layer down asserting wrapError keeps an ABORTED recognisable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Concurrent read-modify-write transactions on a single row must all commit: Spanner aborts the losers and ReadWriteTransaction is expected to retry them transparently. A raw ABORTED surfacing here means something on the path stripped the gRPC status off the error and defeated that retry. Eight writers each append a character to one team name, so the final length also proves no update was lost to a retry replaying stale state. Measured on the emulator, the callback runs 22-26 times for 8 writers, i.e. the aborts and retries are real rather than incidental. Spanner-only for now. Postgres opens transactions at the server default READ COMMITTED and sets no isolation level anywhere, so the same test loses updates there (3 of 8 writers survived) without raising a serialization error there is anything to retry. Tracked separately in #791. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ReadWriteTransaction retries ABORTED for as long as ctx allows, and nothing in the stack supplies a deadline: the HTTP server sets ReadTimeout and WriteTimeout but neither cancels the request context, and there is no timeout middleware. On a hot row a conflict loop can therefore spin indefinitely and pile up goroutines instead of surfacing a clear failure. Apply a 30s default at every site that opens a ReadWriteTransaction, not only Client.Transaction: client.Write and client.Update in db.go each open their own, as does withTransaction. A caller that sets its own deadline keeps it, so this only fills in the unbounded case. The bootstrap user import is per-user rather than one large batch, so it sits well inside the bound. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The emulator serves one transaction at a time, which is exactly what forces the aborts that prove the abort-retry path works. Two workarounds had been suppressing that signal rather than fixing the cause: - moon test-spanner pinned -parallel 1, removing the contention entirely. - CI preferred a real Spanner instance over the emulator when one was configured, which would have hidden the missing retry in #604. Drop the -parallel pin and run the emulator unconditionally. -p 1 stays: the suites share one database and packages like stmttest assert on global list results, so they still need one package at a time. That is test isolation, not contention avoidance. The test-instance steps and the id-token permission are commented out rather than deleted, so the instance can be re-wired quickly if the emulator turns out not to hold. Re-enabling needs the emulator step's if: guard restored too, which is called out inline. Removal is tracked in #793. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…path Two rules that were implicit and cost a CI investigation to rediscover: - Never flatten the cause of an error returned from a transaction. The retry only fires while the gRPC status is still reachable, so %w: %v silently converts a retryable abort into a user-facing error. - A transaction callback must be replayable, because a retry re-runs the whole closure. Also record which spanner.Client methods do not retry ABORTED. BatchWrite and NewReadWriteStmtBasedTransaction are caller-retried by design; nothing uses them today and neither should be adopted without its own retry loop. Document the real test instance as kept but deliberately unwired, with what re-wiring takes and a warning not to reach for it to make a failing test pass, since that hides the aborts these suites exist to catch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
🦋 Changeset detectedLatest commit: 74e3973 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a Spanner-specific retry breakage where session exchange conflict wrapping flattened the underlying ABORTED cause, preventing ReadWriteTransaction from recognizing the error as retryable. The PR also standardizes a default retry bound for Spanner read-write transactions and re-centers CI on the emulator as the contention canary.
Changes:
- Preserve the underlying error chain for session exchange conflicts (
%v→%w) so SpannerABORTEDremains retryable. - Add a default 30s deadline (
boundRetry) at all SpannerReadWriteTransactionentry points to prevent unbounded retry loops. - Update CI/task/docs to run Spanner integration against the emulator at normal parallelism, and add a contention canary integration test.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/storage/v2/testdb/spanner.go | Documents emulator-only CI intent and keeps the instance path explicitly “unwired”. |
| internal/storage/v2/testdb/instance.go | Clarifies that instance provisioning code remains but is currently not configured in CI. |
| internal/storage/v2/session/run_exchange.go | Fixes conflict wrapping to keep the underlying cause in the error chain (%w). |
| internal/storage/v2/session/run_exchange_test.go | Adds regression coverage for preserving retryability through conflict wrapping. |
| internal/storage/v2/dialect/spanner/with_transaction.go | Applies boundRetry to transactions started via withTransaction (client case). |
| internal/storage/v2/dialect/spanner/error_test.go | Adds tests for boundRetry and retryability through wrapError. |
| internal/storage/v2/dialect/spanner/db.go | Applies boundRetry to Write/Update paths that open ReadWriteTransaction. |
| internal/storage/v2/dialect/spanner/client.go | Applies boundRetry to Client.Transaction and defines the 30s default bound. |
| internal/storage/v2/AGENTS.md | Documents the “do not flatten causes in transactions” rule and retry/replay constraints. |
| internal/api/integration_test/transaction_contention_test.go | Adds a Spanner-only contention canary to prove retries under emulator contention. |
| docs/adrs/028-storage-v2-statements-and-dialects.md | Notes that the instance mode is kept but unwired; emulator remains the CI default. |
| CONTRIBUTING.md | Updates contributor guidance to keep the emulator as the retry canary and documents the unwired instance mode. |
| apps/server/moon.yml | Removes -parallel 1 pinning for Spanner tests while retaining -p 1 for cross-package isolation. |
| .github/workflows/ci.yml | Removes the wired test-instance path (kept as commented instructions) and always runs emulator tests. |
| .changeset/spanner-abort-retry.md | Adds a patch changeset for @zitadel/server describing the retryability fix and default deadline. |
Comment on lines
+209
to
+214
| // ...and the abort still reaches Spanner's retry predicate. | ||
| assert.Equal(t, codes.Aborted, status.Code(err), | ||
| "ABORTED was stripped; ReadWriteTransaction will not retry") | ||
| var se *spanner.Error | ||
| assert.ErrorAs(t, err, &se) | ||
| }) |
Comment on lines
+168
to
+172
| require.Error(t, got) | ||
| assert.Equal(t, codes.Aborted, status.Code(got), | ||
| "ABORTED was stripped; ReadWriteTransaction will not retry") | ||
| assert.ErrorAs(t, got, new(*spanner.Error)) | ||
| } |
Comment on lines
+54
to
+68
| for i := range contendingWriters { | ||
| wg.Add(1) | ||
| go func() { | ||
| defer wg.Done() | ||
| <-start // release together so the transactions genuinely overlap | ||
| errs[i] = harness.DB.Transaction(t.Context(), func(ctx context.Context, tx service.Statementer[service.AllStatements]) error { | ||
| current, err := tx.Statements().GetTeamByID(ctx, project.ID, team.ID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| current.Name += "x" | ||
| return tx.Statements().UpdateTeam(ctx, current) | ||
| }) | ||
| }() | ||
| } |
The arm64 emulator build does not round commit timestamps the way the amd64 build CI runs does, so six stmttest subtests and TestJSONSchemaStatements_CRUD see tens of microseconds of drift on a created_at read back after a second statement. Deterministic, unrelated to parallelism, and green in CI. Without this note an arm64 contributor sees 7 red tests with nothing saying they are expected, which teaches them to distrust local Spanner runs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (1)
internal/api/integration_test/transaction_contention_test.go:59
- This canary never verifies that a retry occurred. Releasing eight goroutines together usually creates contention, but scheduling can still let all eight callbacks commit on their first invocation; the final-length assertion would then pass even if retry handling were broken. Count callback invocations and assert the count exceeds
contendingWriters(adding stronger synchronization if necessary), so the test actually proves that an abort was retried as claimed.
<-start // release together so the transactions genuinely overlap
errs[i] = harness.DB.Transaction(t.Context(), func(ctx context.Context, tx service.Statementer[service.AllStatements]) error {
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #788.
Summary
The Spanner CI flakiness from #604 was an application bug, not an emulator limitation.
exchangeConflictininternal/storage/v2/session/run_exchange.gowrapped the conflict sentinel with%wbut formatted the real cause with%v, which flattens it to a string and drops it from the error chain. Spanner'sReadWriteTransactiondecides whether to retry by looking for a gRPC status in the error the callback returns (errors.Asfor*spanner.Error, elsestatus.FromError); with the cause flattened it finds neither and returns immediately. Session exchange runs inside that transaction, so under contention a retryableABORTEDbecame a user-facing conflict.Instrumenting the failing test caught it exactly:
That is the verbatim #604 error, still intact on arrival and destroyed one line later. The fix is
%v→%w.With that fixed, two workarounds that were suppressing the signal are removed:
moon test-spannerno longer pins-parallel 1, and CI no longer prefers a real Spanner instance over the emulator. The emulator's one-transaction-at-a-time limit is what forces the aborts that keep the retry path honest, so it is now the canary rather than the thing being worked around.Three findings corrected the original plan in #788, and are worth a reviewer's attention:
wrapErrorneeded no change. It already preserves the status; that is why the abort survived all the way toexchangeConflict.mapStorageErrorpreservesParent,GenerateNewKeySetreturns a fresh keyset,CreateUserAction.Applyis idempotent. No code change; the requirement is documented instead.-parallel 1, not the real instance. It had been keeping the suite green by removing the contention entirely.Retry is now also bounded. Nothing in the stack supplied a deadline (the HTTP server sets
ReadTimeout/WriteTimeout, but neither cancels the request context, and there is no timeout middleware), so a hot row could spin indefinitely. A 30s default is applied at every site that opens aReadWriteTransaction, not onlyClient.Transaction—client.Write,client.UpdateandwithTransactioneach open their own. A caller with its own deadline keeps it.The real test-instance path is kept but unwired: the Go code in
internal/storage/v2/testdbstill works and the CI steps are commented out rather than deleted, so it can be restored quickly if the emulator turns out not to hold. Removal is tracked in #793.Validation
go build ./...,go vet -tags spanner_integration ./...,go test ./internal/...— clean.-parallel 32: 5 consecutive green runs. The same command failed on the first run before the fix.TestTransactionContentionon the emulator: 8 concurrent writers to one row produce 22-26 callback invocations, all committing, final length exact — so the aborts and retries are real, not incidental.0x2Unknown instead of0xaAborted, chain flattened todomain.Error) and pass with it.go vet -tags spanner_integration ./...), so the history bisects.TestTransactionContentionpassed there (0.49s), so the canary holds on CI's amd64 runners and not just locally.Release notes / changeset
Changeset:
.changeset/spanner-abort-retry.md— patch for@zitadel/server. Sign-in no longer intermittently fails with a session exchange conflict on Spanner, and read-write transactions now run under a default 30s deadline.Notes
One defect found while verifying this, outside its scope and filed separately:
Also worth knowing for anyone running these suites on Apple Silicon: 7 subtests in
stmttestandTestJSONSchemaStatements_CRUDfail locally on arm64 and pass in CI, because the arm64 emulator build rounds commit timestamps differently from the amd64 build CI runs. Deterministic, unrelated to parallelism, and nothing broken in CI or shipped. Investigated and closed as not planned (#792); the caveat and its workaround are documented inCONTRIBUTING.mdso the next arm64 contributor is not left guessing at 7 red tests.-p 1is retained in the moon task deliberately. The suites share one database and packages likestmttestassert on global list results, so they still need one package at a time; that is test isolation, not contention avoidance. Removing it surfaces cross-package leakage (aListProjectsassertion seeing another package's rows).🤖 Generated with Claude Code