fix(server,proxy): portable REPO_ROOT + flush queued usage-worker messages on shutdown - #63
Open
doanhv wants to merge 2 commits into
Open
Conversation
added 2 commits
July 23, 2026 22:18
graceful-shutdown.integration.test.ts hardcoded REPO_ROOT = "/Users/brain/Coding/snipeship/ccflare" (the original author's local path, used as Bun.spawn's cwd. On any other checkout this directory does not exist, and Bun's posix_spawn fails with a misleading ENOENT attributed to the bun binary rather than the cwd. Reuse the existing portable helper from ./test-helpers/workspace.ts (REPO_ROOT computed via import.meta.dir, already used by sibling integration tests in this same app) instead of a second, hardcoded definition. Note: this test still fails after this fix, but for a separate, unrelated reason (a real bug in packages/proxy/src/usage-worker.ts's terminateGracefully(), tracked separately) -- this commit only removes the path-portability failure mode, confirmed by an independent review that reproduced both the old ENOENT crash (before) and the new, different assertion failure (after) via direct execution. EOF )
terminateGracefully() cleared queuedMessages synchronously and unconditionally, even when the worker was not yet ready and those messages had never been sent -- permanently losing pending usage/analytics-log writes whenever shutdown raced with worker startup. handleWorkerMessage()'s shuttingDown early-return also sat before the ready-message branch, so a ready signal arriving during the shutdown window was ignored, making the loss unrecoverable once shutdown began. terminateGracefully() now waits (bounded by readyTimeoutMs) for the worker to become ready before flushing, only dropping (with a logged warning) whatever is still queued afterward. handleWorkerMessage's ready branch now unblocks that wait unconditionally, while still only auto-flushing when not shutting down. forceTerminate() gets a best-effort, non-blocking flush instead of an unconditional wipe. Two further races were found and fixed during independent review: forceTerminate() racing an in-flight terminateGracefully() wait used to leave that promise unsettled for the full timeout and then silently resolve instead of reject; and a standalone forceTerminate() could leak stale error state into a later, unrelated terminateGracefully() call. Both are covered by dedicated regression tests. Second commit on this branch: builds on the REPO_ROOT path fix already here, since apps/server/src/graceful-shutdown.integration.test.ts only passes reliably once both issues are fixed.
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.
Two stacked bugs found while investigating why graceful-shutdown.integration.test.ts fails: (1) REPO_ROOT was hardcoded to the original authors local machine path, used as Bun.spawns cwd -- fails with a misleading ENOENT on any other checkout. Fixed by reusing the repos own pre-existing portable helper (test-helpers/workspace.ts). (2) After that fix, the test still failed with a different signature (exitCode=1): usage-worker.ts's terminateGracefully() wiped queuedMessages synchronously without ever trying to flush them if the worker was not ready, permanently losing pending usage/analytics-log writes on shutdown; compounded by handleWorkerMessage()'s shuttingDown early-return sitting before the ready-message branch. Fixed with a bounded wait-for-ready-then-flush before dropping (with a logged warning) whatever remains queued. Two further races were found and fixed during independent review: forceTerminate() racing an in-flight terminateGracefully() wait used to leave that promise unsettled for the full timeout then silently resolve instead of reject; and a standalone forceTerminate() could leak stale error state into a later, unrelated terminateGracefully() call. Both covered by dedicated regression tests. Test plan: graceful-shutdown.integration.test.ts passes reliably now (was deterministic failure before); 3 new usage-worker.test.ts unit tests, stable across repeated runs; full workspace typecheck clean.