[pull] main from czlonkowski:main - #108
Merged
Merged
Conversation
…fore, fix shutdown flush (#980) * chore(telemetry): rotate publishable key, 60s flush, drop workflow_before Rotate the bundled telemetry key to the new Supabase publishable key format (sb_publishable_...). SUPABASE_ANON_KEY still overrides it. Raise BATCH_FLUSH_INTERVAL from 5s to 60s. Queue thresholds and the beforeExit/SIGINT/SIGTERM flush are unchanged, so short stdio sessions still ship their batch on exit. Stop sending workflow_before on mutation records. It was a full second copy of the workflow on every update for a snapshot nothing queried. The before snapshot is still built locally for deduplication, the meaningful-change check, and the before hashes, which continue to identify the prior state. Conceived by Romuald Członkowski - www.aiadvisors.pl/en * chore(telemetry): tighten mutation-record comments and test assertion Comment polish on the removed workflow_before path: state that the before snapshot never leaves processMutation and that its hashes are computed over the sanitized form, so they stay comparable with rows recorded earlier. The new test now asserts workflowAfter survives intact and the structural before-hash is present, rather than just checking the field exists. No behavior change. Conceived by Romuald Członkowski - www.aiadvisors.pl/en * fix(telemetry): flush queued data on shutdown instead of relying on beforeExit Every shutdown path ends in process.exit(), which does not emit 'beforeExit', so the batch processor's exit handler never ran and anything queued since the last interval flush was dropped. At a 5s interval that was a small window; at 60s it loses most short sessions, and single-mutation sessions entirely, since mutations auto-flush only from the second queued record onward. Add telemetry.flushBeforeExit(): a bounded, non-throwing final flush capped by SHUTDOWN_FLUSH_TIMEOUT_MS so an unreachable backend cannot delay or fail an exit. Awaited from the MCP server's shutdown() (which covers both stdio entry points) and from the single-session HTTP server's shutdown(), which closes per-session servers directly rather than going through that path. Corrects the CHANGELOG claim that short stdio sessions already shipped their batch on exit — they did not. Conceived by Romuald Członkowski - www.aiadvisors.pl/en * chore(telemetry): address Copilot review on the shutdown flush Document that ANON_KEY now holds a publishable key and why the field keeps its name: it mirrors the SUPABASE_ANON_KEY environment variable that overrides it, which is a documented public contract. Note in flushBeforeExit that its deadline stops the await rather than cancelling the flush, and that request-level bounding already comes from telemetryFetch's FETCH_TIMEOUT_MS abort. Replace the wall-clock assertion in the deadline test with fake timers, so it proves the bound (not settled at 4999ms, settled at 5000ms) instead of depending on CI scheduling. Conceived by Romuald Członkowski - www.aiadvisors.pl/en * fix(telemetry): keep test runs offline and harden the shutdown flush Awaiting the flush in shutdown() exposed that nothing disabled telemetry in tests: with no config file present the first-run default is enabled, so integration tests that construct a server both shipped rows to the production project and blocked each teardown on a real round-trip. Measured on tool-invocation.test.ts (20 shutdowns) against an unreachable host: 1.12s before the flush, 31.39s after, 1.61s with N8N_MCP_TELEMETRY_DISABLED set in vitest.config.ts. The config-manager suite now saves, clears, and restores the three opt-out variables so it still exercises the enabled-by-default path deterministically. Lower SHUTDOWN_FLUSH_TIMEOUT_MS to 2000 so it cannot tie with a caller's shutdown budget (the integration helper races close against 3000ms, and two equal timeouts could abandon its transport cleanup). Move the flush ahead of the initialization await in shutdown(): telemetry needs no database, so an initialization that never settles should not also cost the queued events. Cover the two test gaps: the reject case now rejects from flush() itself rather than the batch processor, which swallows its own errors and left the catch block unexercised, and a new case pins that a shutdown never initializes telemetry when it was never initialized. Conceived by Romuald Członkowski - www.aiadvisors.pl/en * fix(telemetry): widen the shutdown flush budget and pin the call site SHUTDOWN_FLUSH_TIMEOUT_MS at 2000 exactly equalled FETCH_TIMEOUT_MS, and a batch sends events, workflows and mutations as three sequential requests — so on a slow link only the first was guaranteed to land, and mutations, which go last and are the rarest records, were the first casualty. 2500 keeps the budget above the per-request cap while staying below the 3000ms shutdown budget callers allow themselves. Guard the flush call in shutdown() like every other cleanup step there: telemetry must never change a shutdown's outcome, which for src/mcp/index.ts would mean exit code 1 and skipped stdin teardown. Add tests pinning the call site itself — that shutdown() awaits the flush, that it does so ahead of the database initialization await, and that a rejecting flush still shuts down cleanly. Nothing failed before if the call was simply deleted. Also stop two tests from being able to go vacuous later: the opt-out restore in config-manager.test.ts no longer assigns an undefined value (which Node coerces to the string "undefined"), and the never-initialized case asserts the mocked config manager still reports enabled, since that is what gives it teeth. Conceived by Romuald Członkowski - www.aiadvisors.pl/en * test: complete the telemetry mock and assert shutdown cleanup ran The shutdown test file mocked the telemetry barrel with 7 of the 10 methods reached through it — handlers-n8n-manager imports the same barrel, so trackEvent, trackWorkflowCreation and trackWorkflowMutation were missing. The three existing tests never reach them, but the next test added to this file that exercises a workflow tool would have died with "is not a function" instead of failing meaningfully. The rejecting-flush case now also asserts the cleanup past the flush ran, so "shuts down cleanly" is checked rather than inferred from the promise resolving. Conceived by Romuald Członkowski - www.aiadvisors.pl/en * chore(telemetry): clear the shutdown deadline timer on every path Move clearTimeout into a finally block so a rejecting flush cannot leave the deadline pending. Harmless while the timer is unref'd, but it would surface under fake timers. Conceived by Romuald Członkowski - www.aiadvisors.pl/en * chore(telemetry): stop the probe scripts duplicating the backend constants Both scripts carried their own copy of the URL and key, which is why this rotation had to touch three files and why one of them still held a key from an earlier rotation. They now import TELEMETRY_BACKEND and resolve the same env overrides the runtime does, so they always probe the credentials the package actually ships with. The key now exists in exactly one place in source. Conceived by Romuald Członkowski - www.aiadvisors.pl/en * docs(telemetry): state the shutdown flush bound accurately The comment claimed nothing outlives the deadline by more than one request. A batch sends events, workflows and mutations as separate sequential requests, so a deadline that fires during the first still leaves the rest to be issued: the bound is per request, not total. What makes that acceptable is that every caller exits immediately after this returns, which the comment now says instead. Conceived by Romuald Członkowski - www.aiadvisors.pl/en
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )