fix(push): give the timeplanning sender its own named Firebase app - #1694
Conversation
FirebaseApp.DefaultInstance is process-wide. TimePlanning.Pn and BackendConfiguration.Pn are loaded as plugins into the same eFormAPI.Web process and each holds the credential for a different Firebase project, so whichever initialises first owns the default instance and every other sender silently pushes through the first one's project. Every token then returns SENDER_ID_MISMATCH, which this service reads as a credential fault and retries forever without ever surfacing an error. Three tests, all against the real constructor: - the sender creates an app named for timeplanning and never claims the process-wide default; - a second initialisation reuses that app instead of calling FirebaseApp.Create again (Create THROWS on a taken name, and the constructor swallows that into "push disabled") - the deterministic form of the concurrent-first-request race; - eight threads racing the very first initialisation all end enabled. Expected to fail until the sender is moved off the default instance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HknsjYQGfBzXpCtSXgU8Nk
There was a problem hiding this comment.
🟡 Changes recommended
The new tests assert behavior that the current production PushNotificationService implementation does not provide (still uses FirebaseApp.DefaultInstance/FirebaseMessaging.DefaultInstance), and the concurrency test currently shares EF Core DbContexts across threads, risking flakiness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds test coverage intended to validate that TimePlanning.Pn’s PushNotificationService owns a dedicated, named Firebase app (and remains enabled under concurrent initialization) to avoid cross-plugin credential contamination in the shared eFormAPI.Web process.
Changes:
- Adds fixture-level Firebase app cleanup and a synthetic service-account configuration helper for exercising real Firebase initialization paths.
- Adds three new tests asserting a named Firebase app is created/reused and that initialization remains enabled under concurrent “first request” conditions.
- Introduces a minimal in-test
ILoggerimplementation to assert initialization produced no errors.
File summaries
| File | Description |
|---|---|
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/PushNotificationServiceTests.cs | Adds new tests and helpers around Firebase app ownership, re-initialization, and concurrent initialization behavior. |
Review details
Suppressed comments (2)
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/PushNotificationServiceTests.cs:292
- To keep the concurrency test valid without cross-thread DbContext usage, create and dispose the TimePlanningPnDbContext inside each racing thread before constructing PushNotificationService.
{
startLine.SignalAndWait();
_ = new PushNotificationService(contexts[i], loggers[i]);
}))
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/PushNotificationServiceTests.cs:308
- After moving DbContext creation into each thread, this disposal loop should be removed (each thread disposes its own context).
foreach (var context in contexts)
{
await context.DisposeAsync();
}
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| _ = new PushNotificationService(TimePlanningPnDbContext!, logger); | ||
|
|
||
| Assert.Multiple(() => | ||
| { | ||
| Assert.That(FirebaseApp.GetInstance(ExpectedFirebaseAppName), Is.Not.Null, | ||
| $"this sender must own a Firebase app named '{ExpectedFirebaseAppName}'"); | ||
| Assert.That(FirebaseApp.DefaultInstance, Is.Null, | ||
| "FirebaseApp.DefaultInstance is shared with every other plugin in " | ||
| + "eFormAPI.Web; claiming it cross-contaminates Firebase credentials"); |
| var contexts = Enumerable.Range(0, racers) | ||
| .Select(_ => CreateTimePlanningPnDbContext()).ToList(); | ||
| var startLine = new Barrier(racers); |
…fault The sender now owns a FirebaseApp named "microting-time" and pushes via FirebaseMessaging.GetMessaging(app). Previously it claimed FirebaseApp.DefaultInstance, which is process-wide and shared with every other plugin in the eFormAPI.Web host - BackendConfiguration.Pn already has its own sender and a third is coming. Whichever plugin initialised first owned the default, and every other sender then pushed through that project's credential; every token came back SENDER_ID_MISMATCH, which PruneSenderIdMismatchesAsync reads as a credential fault and leaves alone, so the send was retried forever and never surfaced as an error. Also fixes the pre-existing initialisation race. The old create had no lock and no double-check, so two concurrent first requests could both pass the null test; the loser's FirebaseApp.Create threw FirebaseAppAlreadyExistsException, the constructor's catch turned that into _isEnabled = false, and that request silently sent nothing. EnsureFirebaseApp now double-checks FirebaseApp.GetInstance(name) - which returns null rather than throwing when absent - inside a lock, mirroring AdhocReminderJob.EnsureFirebaseApp in the service plugin. _isEnabled is replaced by the _firebaseApp field it stood for, so the service cannot be "enabled" without an app to send through. Credential key, scoped lifetime, min-build gating, AppId filter, prune set and the silent skip when unconfigured are all unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HknsjYQGfBzXpCtSXgU8Nk
Production:
- #nullable enable on the service and its interface. The _firebaseApp
gate is the whole safety argument of the previous commit, and without
a nullable context the compiler was ignoring the annotation (CS8632).
- The comments named FirebaseAppAlreadyExistsException, which does not
exist in FirebaseAdmin 3.6.0 - Create throws a plain ArgumentException
("FirebaseApp named ... already exists"). Anyone narrowing the catch on
the strength of those comments would have written code that cannot
compile.
- FirebaseInitLock is private to this assembly, so it cannot serialise a
creator outside it. EnsureFirebaseApp now catches that ArgumentException
and re-reads the registry, so its postcondition ("an app of this name
exists when this returns") holds no matter who won - and rethrows when
the app really is absent, so an unrelated ArgumentException is not
swallowed.
- CredentialFactory.FromJson<ServiceAccountCredential> replaces the
obsolete GoogleCredential.FromJson (CS0618). Pinning the generic also
fails fast into the constructor's catch when the configured JSON is not
a service-account key, and makes the "mirrors AdhocReminderJob" pointer
true on the one line it is about.
- GetMessaging is hoisted out of the per-token loop; it took
FirebaseAdmin's global lock once per device token to re-derive a value
that cannot change. The load-bearing "never DefaultInstance" comment
moves with it.
- Constructor init extracted to ResolveFirebaseApp, which drops two
redundant "= null" assignments to a readonly field and unindents the
happy path. The skip log now says "push disabled" rather than "not
configured", which was misleading when initialisation had thrown.
Tests:
- The race test could kill the test host: the constructor's DB read sits
outside its own try, and an exception escaping a bare Thread tears down
the process - shard f would have died with no attribution instead of
one red test. Failures are collected and asserted. Barrier and Join are
now bounded (nothing in this suite sets a timeout, so a stranded racer
would have burned GitHub's 360-minute default), and the Barrier is
disposed.
- Shared AssertOwnsNamedAppAndNotTheDefault: two of the three tests
asserted the most important invariant in the change with no failure
message at all.
- The synthetic service-account key is generated once per run instead of
per test, and the ten-line comment restating the production doc is now
a pointer to it - two copies of one rationale drift, and the test copy
goes stale first.
- RecordingLogger loses an unused type parameter; a note records that
this is the only fixture touching the process-wide FirebaseApp registry
while fixtures run in parallel.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HknsjYQGfBzXpCtSXgU8Nk
Problem
PushNotificationServicecreates the unnamed default Firebase app:FirebaseApp.DefaultInstanceis process-wide.TimePlanning.PnandBackendConfiguration.Pnare both loaded as plugins into the sameeFormAPI.Webhost process, and a third sender for flutter-eform is about to be added toBackendConfiguration.Pn. Whichever plugin initialises first wins the default instance; every other sender then silently sends through the first one's Firebase project. Different projects, so every token comes backSenderIdMismatch— whichPruneSenderIdMismatchesAsynccorrectly reads as a credential fault and leaves alone, so it is retried forever and never surfaces as an error.Second, pre-existing bug
The create has no lock and no double-check. Two concurrent first requests can both pass the null check; the second
FirebaseApp.CreatethrowsFirebaseAppAlreadyExistsException, which the constructor's catch turns into_isEnabled = false— that request then silently sends nothing.Fix
microting-time(documentedconst), created withFirebaseApp.Create(options, name).FirebaseMessaging.GetMessaging(app), neverFirebaseMessaging.DefaultInstance.FirebaseApp.GetInstance(name), which returnsnullwhen absent rather than throwing — mirroringAdhocReminderJob.EnsureFirebaseAppin eform-service-backendconfiguration-plugin. Concurrent first requests can no longer disable push.Everything else is unchanged: same credential key, same scoped lifetime, same min-build gating, same
AppIdfilter, same prune set, same silent skip when unconfigured.Tests
Three new tests in
PushNotificationServiceTests(already covered by CI shardf), all driving the real constructor against a synthetic service-account key:FirebaseApp.DefaultInstancenull;No existing test asserted on the default instance.
🤖 Generated with Claude Code
https://claude.ai/code/session_01HknsjYQGfBzXpCtSXgU8Nk