oncetask: add Firestore integration tests and fix lifecycle bugs - #17
Open
anish749 wants to merge 2 commits into
Open
oncetask: add Firestore integration tests and fix lifecycle bugs#17anish749 wants to merge 2 commits into
anish749 wants to merge 2 commits into
Conversation
Add an integration test suite (build tag "integration", testcontainers-go + Firestore emulator) covering every public Manager method, handler strategy, retry policy, recurrence variant, cancellation path, env isolation guarantee, panic recovery, and context helper. CI gains a separate job that runs `go test -tags=integration -race` against the emulator started inside the test process. Bugs found and fixed in the same change: - cleanup function returned by NewFirestoreOnceTaskManager did not wait for worker goroutines to exit; tests racing the next manager would see "Transaction lock timeout" against the emulator. Cleanup now signals via a dedicated stop channel (so in-flight transactions finish naturally, releasing locks) and blocks on a WaitGroup. - CancelTasksByIds did not signal the affected task type; cancelled tasks waited up to 60s for the next polling tick. Now it calls evaluateNow per affected type. - Retried tasks waited up to 60s for the polling tick even when the retry delay was milliseconds. completeBatch now schedules a wakeup at the earliest retry waitUntil.
…orcing exit Building on #18, cleanup() now signals workers via a dedicated stop channel and waits up to 5s for them to exit between iterations (letting any in-flight Firestore RPC complete naturally) before falling back to context cancellation. Cancelling a transaction mid-flight leaves the Firestore emulator holding row/index locks for several seconds while it processes the abort. With the integration test suite running tests back-to-back against a shared emulator, the next test reliably tripped "Transaction lock timeout" on its first CreateTask. The graceful path sidesteps this; the 5s fallback ensures unreachable endpoints (used by the existing cleanup_test.go) still terminate promptly.
anish749
force-pushed
the
worktree-firestore-integration-tests
branch
from
May 9, 2026 13:25
94231dd to
a680c7c
Compare
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
1 participant
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.
Problem
The library shipped with zero integration tests — every existing
*_test.goexercised in-memory logic only. Real Firestore behaviour(lease acquisition, COUNT aggregation, BulkWriter, env isolation,
recurrence spawning, cancellation handlers, retry timing, etc.) was
untested end-to-end.
Writing those tests also surfaced three real bugs that affected
production semantics, not just test ergonomics — so they're fixed here
too.
Solution
Integration tests
integration, gatedseparately from unit tests. Run via
go test -tags=integration ./....(
shopkeeper): emulator boots once viatestcontainers/modules/gcloud(
cloud-sdk:506.0.0-emulators— needed for the multi-fieldinequality queries in
firestore_queries.go), shared across alltests. Each test gets its own
ONCE_TASK_ENVso data is isolatedon the shared collection.
variants, cancellation entry points, delete operations).
Managermethod, everyHandlerOption, all threeexecution strategies (Concurrent, OnePerResourceKey, AllPerResourceKey),
exponential / fixed / no-retry / custom policies, scheduled tasks,
recurrence (DAILY / WEEKLY / COUNT / ExDate / MONTHLY / SECONDLY),
cancellation with and without cleanup handler, reset, delete, env
isolation (reads + handler claims + write rejection), panic recovery,
and context helpers.
CI
New
integrationjob in.github/workflows/ci.ymlruns the fullsuite with
-raceon Ubuntu (testcontainers uses the runner's Docker).Bugs fixed in the same PR
Cleanup didn't wait for worker goroutines.
NewFirestoreOnceTaskManagerreturned a cleanup that just cancelledthe context. Cancelling mid-transaction left the emulator holding
row/index locks for several seconds; the next test's
CreateTasktripped
Transaction lock timeout. Fix: introduce astopchannelchecked between iterations (so in-flight transactions complete and
release locks naturally) plus a
sync.WaitGroupthat cleanupblocks on. Cancelling the user-provided ctx is still the abrupt
path.
CancelTasksByIds didn't notify workers.
Cancelled tasks sat until the next 1-minute polling tick before the
cancellation handler could run. Fix: track the affected task types
and call
evaluateNowfor each.Retried tasks waited up to 60s for the next tick.
Even with a 100 ms retry delay, the worker would sleep on its
minute-long ticker after scheduling the retry. Fix:
completeBatchnow computes the earliest retry waitUntil and schedules a wakeup at
that time via a goroutine that signals
evaluateChan.Context
All 38 integration tests pass under
-racein ~30s on a freshemulator. Existing unit tests still pass.
golangci-lintis cleanboth with and without
-tags=integration.