Repository navigation
Add GET /healthz and redact bin store errors - #249
Conversation
GET /healthz pings the bin store and returns a redacted error. A second workerd socket error no longer restarts the isolate. Co-authored-by: Jared Wray <me@jaredwray.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #249 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 41 44 +3
Lines 1436 1723 +287
Branches 296 385 +89
==========================================
+ Hits 1436 1723 +287 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d23884c89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Short alphabetic tokens are substrings of words like "username". | ||
| if (/^[a-z]+$/i.test(secret) && secret.length < 8) { | ||
| return message; |
There was a problem hiding this comment.
When a URI contains a short alphabetic username, password, or host, this branch leaves that value in public health responses and logs. For example, mongodb://bob:foo@mongo/db does not redact bob, foo, or mongo from a driver error. This breaks the stated rule that these values never appear.
Useful? React with 👍 / 👎.
| const reset = (store: object) => { | ||
| keyv = undefined; | ||
| openedFor = undefined; |
There was a problem hiding this comment.
With concurrent MongoDB failures, the first failure clears the cached client and a later request opens a replacement. A later failure from the old store clears that replacement because reset does not verify the active store. The next request opens another pool and leaves the replacement pool open, which can exhaust connections during an outage.
Useful? React with 👍 / 👎.
| if (typeof source.on === "function") { | ||
| source.on("error", (error) => { | ||
| noteFailure("keyv.error", uri, error, reset); | ||
| }); |
There was a problem hiding this comment.
When KeyvMongo emits an error outside an awaited store method, this listener logs it but Keyv's forwarding listener still throws it. The added test confirms that the original error escapes from emitError. A background network error can therefore abort the Worker isolate instead of only resetting the failed client.
Useful? React with 👍 / 👎.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix and feature.
GET /healthzreports bin storage. Store failures return a redacted driver error. A failed Mongo connect does not abort the Worker.Finding
A well-formed
mongodb+srvURI does not throwInvalid connection stringunder workerd. The driver parses it and then reaches DNS.Invalid connection stringstill means a second@or another unencoded reserved character in the username or password. This PR does not change the secret and does not percent-encode it. SRV DNS works for a correct URI. No seed-list URI, Atlas Data API, or extra TCP compat flag is required.What changed
GET /healthzis on the Node server and the Worker.{status:'ok', bins:{store:'mongo'|'memory', connected:true, latencyMs, uriPresent, scheme}}after a real ping (adminpingor a cheap memory read).{status:'error', bins:{store:'mongo', connected:false, uriPresent, scheme, error:{name, message, code}}}when the ping fails.uriPresentis a boolean.schemeismongodbormongodb+srv. The body and the logs never include the URI, username, password, or host. Connection-string fragments are removed from the error message.source: mockhttp.bin-store) with the same redaction.POST /binsincludes the safe driver message.node:netemitserrortwice for one failed socket. The driver listens withonce, so the second emit has no listener and aborts the isolate. The guard drops that second emit for every message, including a proxy failure, a TLS failure, and a reset. The first emit still reaches the driver.POST /binsthen returns the app 500.Docs:
site/docs/bins.md,site/docs/how-to-deploy/cloudflare.md, andsite/docs/library.md.Verification
pnpm test: 561 tests passed. Line coverage 100% (1435/1435).pnpm buildcompleted on the previous commit. This follow-up changes only the socket guard and its unit test.mongodb://127.0.0.1:1/mockhttp:GET /healthzis 503, thenPOST /binsis the app 500. The isolate does not restart.mongodb+srv://...:GET /healthzis 503 and the body does not containInvalid connection string.@: the body containsInvalid connection stringand the percent-encode hint, and does not contain the password or host.Test audit
No test was deleted or dropped. The socket guard test was rewritten: it now proves a second
erroris dropped for both the proxy message and a TLS handshake message, while the first emit still reaches aoncelistener.Do not merge, release, or deploy this PR.