Fix records/query auth mismatch and add bounded shutdown - #77
Merged
Conversation
POST /records/query required auth while docs said Optional; align to GET /records's anonymous-friendly behavior (it's a superset of the same query surface) and pin it with a test. shutdown() previously waited on server.close() indefinitely, so a client holding a keep-alive connection open could block it forever. Add a configurable SHUTDOWN_TIMEOUT_MS deadline, after which open connections are forced closed so cleanup still runs, and flush the stack on the fatal-startup-error path too. The shutdown sequence is extracted into its own module so it's unit-testable without invoking process.exit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RgFKoffjrpKGCdEYX551LY
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.
Summary
Closes the two items still open in #49 (the other four were already fixed by prior work, per the issue's own follow-up comment):
POST /records/queryauth doc mismatch: the route required auth whiledocs/api.mdlisted it asOptional. Aligned code to the docs (removedrequireAuth()) rather than the reverse, sinceGET /recordsis already anonymous-friendly and/records/queryis documented as a superset of the same query surface. Pinned with a test mirroring the existingGET /recordsanonymous-query test. Updateddocs/deployment.md's two mentions of anonymous-reachable read routes to includePOST /records/query.shutdown()waited onserver.close()with no deadline, so a client holding a keep-alive connection open could block it indefinitely. Added a configurableSHUTDOWN_TIMEOUT_MS(default 10s, same pattern asQUERY_TIMEOUT_MS), after which open connections are forced closed viacloseAllConnections()so cleanup still runs. Also addedstack.flush()to the fatal-startup-error path, guarded for the case where the crash happens before the stack context exists. The shutdown sequence is extracted intosrc/shutdown.tsso it's unit-testable without touchingprocess.exit.Test plan
pnpm test— 336 tests pass, including new coverage for anonymousPOST /records/query,SHUTDOWN_TIMEOUT_MSconfig validation, and the shutdown handler's timeout/no-timeout/no-closeAllConnectionspathspnpm typecheckpnpm lintpnpm format:checkGenerated by Claude Code