fix(test): make httpapi-exercise 500s carry their cause (TKT-372, fb 183) - #60
Open
sepo-eng wants to merge 1 commit into
Open
fix(test): make httpapi-exercise 500s carry their cause (TKT-372, fb 183)#60sepo-eng wants to merge 1 commit into
sepo-eng wants to merge 1 commit into
Conversation
…183)
An effect-mode failure reported {"name":"UnknownError","ref":"err_xxxxxxxx"} and
nothing else. On TKT-323 that turned a one-glance answer -- Unbound layer node:
@opencode/InstanceBootstrap -- into a dedicated diagnosis assignment.
The cause was never lost. The error middleware already logs it via
Effect.logError, and feedback 183 attributed the loss to backend.ts's
disableLogger:true. That attribution is wrong: disableLogger only skips adding
HttpMiddleware.logger, the per-request access log. Built both ways against the
same dying route, the Cause prints either way.
The actual suppressor is core's observability layer, which builds with
Logger.layer(..., { mergeWithExisting: false }) over Logging.loggers() -- and
loggers() returns the file logger alone unless OPENCODE_PRINT_LOGS is set. So the
Cause went to opencode.log while the harness printed an opaque ref.
The exerciser now defaults OPENCODE_PRINT_LOGS=1 and OPENCODE_LOG_LEVEL=ERROR
where it already isolates OPENCODE_DB and the XDG roots, before the app modules
load. Errors only: a green run adds four lines, from the integration scenarios
that deliberately assert 500, and the logged ref matches the ref in the response
body so a failure correlates to its cause.
Verified against a deliberately broken route: with the default the failure prints
the full Cause with file and line; with OPENCODE_PRINT_LOGS=0 the same failure
reproduces the reported symptom exactly. All three modes stay green, 237/237.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KbNYZHcnxRCknFmhhPHe7L
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
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.
First item of TKT-372, which orders
183 first (diagnosability unblocks the rest).The reported problem is real; its stated cause is not
An effect-mode failure gave you
{"name":"UnknownError","ref":"err_xxxxxxxx"}and nothing else, anywhere. Feedback 183 pinned that onbackend.ts:79's{ disableLogger: true }.I tested that before implementing it, and it is wrong.
disableLoggeronly controls whetherHttpMiddleware.logger— the per-request access log — is composed in. Building the same dying route both ways prints the middleware'sEffect.logErrorin both arms:Had I implemented the suggested fix, it would have merged looking correct and changed nothing.
The actual suppressor
packages/core/src/observability.tsbuilds logging asLogger.layer([...Logging.loggers()], { mergeWithExisting: false })— it replaces the console logger — andLogging.loggers()returns the file logger alone unlessOPENCODE_PRINT_LOGS === "1". The Cause was never discarded; it was written toopencode.logwhile the harness printed an opaque ref.The fix
Default
OPENCODE_PRINT_LOGS=1andOPENCODE_LOG_LEVEL=ERRORin the exerciser'senvironment.ts, alongside theOPENCODE_DBand XDG isolation that already has to happen before the app modules load. Both are??=, so an explicit value still wins.Error level only, so this adds one line per defect and nothing else.
Verified against a real failure
Forcing a route to die (
throwin theglobal.healthhandler):ref=err_a8d0807bmatches therefin the 500 body, so a failure correlates to its cause:OPENCODE_PRINT_LOGS=0— reproduces the reported symptom exactly: theFAILline, the opaque ref, no cause.(First attempt at this probe patched
/global/healthwhile running the/api/healthscenario, so the arm proved nothing and the route passed. Reported here because the corrected pairing is what the evidence above rests on.)All three modes green: 237/237 pass, 0 fail each for coverage, auth, and effect.
Noise, measured rather than promised
A green effect-mode run adds exactly four ERROR lines, all from integration scenarios that deliberately assert 500:
v2.integration.connect.keyKey method not found: missingv2.integration.connect.oauthOAuth method not found: missing/missingv2.integration.attempt.statusOAuth attempt not found: con_missingv2.integration.attempt.completeOAuth attempt not found: con_missingThose four routes raise plain
Errors instead of returning a typed 404, which is why they land in the defect path at all. This change makes that visible; it does not cause it, and it is out of scope here.Not in this PR
The rest of TKT-372 (146, 147, 189, 190, and verifying whether 146/147 explain 145/151) — the ticket stages 183 first deliberately. One finding for whoever takes the next slice: feedback 146's suggested remedy is now known-harmful. It proposes per-scenario fresh memoMaps, but TKT-349 moved
runtime.tsthe opposite way, onto the shared production MemoMap, because a freshLayer.makeMemoMapUnsafe()there split a process singleton in two (runtime.ts:25-32records it first-hand). The masking concern 146 raises is still real; that particular fix for it is not available.🤖 Generated with Claude Code
https://claude.ai/code/session_01KbNYZHcnxRCknFmhhPHe7L