fix: capture S3 diagnostics and stop logging presigned URLs on SQL cache failures - #114
Draft
tkislan wants to merge 3 commits into
Draft
fix: capture S3 diagnostics and stop logging presigned URLs on SQL cache failures#114tkislan wants to merge 3 commits into
tkislan wants to merge 3 commits into
Conversation
…che failures `upload_sql_cache` reported failures using only the string from `raise_for_status()`. For presigned S3 uploads that discarded the one thing identifying the cause (the response body) while including the one thing that should never reach logs (the presigned URL, which carries X-Amz-Credential and X-Amz-Security-Token). The unique URL in every message also meant no two occurrences shared a message string, so downstream error tracking could not group them. - Extract S3's <Code>/<Message>/<Expires>/<ServerTime> plus x-amz-request-id rather than the exception string. Only those four elements are read; <CanonicalRequest>, <StringToSign> and <AWSAccessKeyId> echo the signed query string and are never logged. - Redact credential-bearing material from any text destined for a log. The primary defence strips the query string off anything URL-shaped, including the scheme-less path-only form urllib3 puts in connection errors - so network failures, not just HTTP ones, are covered. - Carry the URL's issue time in a SqlCacheUpload NamedTuple and log seconds_since_url_issued beside url_expires_in, making an expiry-driven failure self-evident. - Use constant log messages with all variable data in `extra`, at all four sites in the module. - Give serialization failures a distinct `failed_to_serialize_cache` cause, and log only the exception type for them - the message quotes user column names. - Fetch the cached object explicitly instead of handing the URL to pandas, which fetched it with urllib and raised before S3's error body was ever read. The parquet/pickle fallback is preserved and now downloads once instead of twice. - Bound the connect phase of both transfers at 5s; read is left unbounded so slow but healthy transfers of large cache objects are unaffected. Cache failures remain swallowed and can never fail the user's query. tests/unit/test_sql_caching.py goes from 26 tests in 31.1s to 74 in 1.4s - the former suite had a test making a real network call to localhost:19456 whose mocks were provably dead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015xJczfWE4mq3ux7ZU9ZeU9
|
📦 Python package built successfully!
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #114 +/- ##
==========================================
+ Coverage 74.42% 74.92% +0.50%
==========================================
Files 95 96 +1
Lines 5704 5806 +102
Branches 850 863 +13
==========================================
+ Hits 4245 4350 +105
+ Misses 1182 1177 -5
- Partials 277 279 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
|
🚀 Review App Deployment Started
|
Applies review findings on the SQL cache diagnostics: - Bound the read phase of object store requests. The previous comment held that a read timeout would abort slow but healthy transfers; requests applies it per received chunk, not to the transfer as a whole, so it only ever aborts a connection that has gone silent. - Read a failed download's body off the wire instead of buffering it whole, and accumulate chunks up to the cap. A chunked response answers with one HTTP chunk per read however large a size is asked for, so reading once lost <Code> and <Message> entirely on a small-chunked error body. - Measure the presigned URL's age when the PUT starts rather than when the failure is logged. S3 checks a signature on arrival, so folding the transfer into the age made a slow upload read back as an expired URL. Transfer time is now reported separately as upload_duration_seconds. - Cut an over-encoded query string off object_path before logging it. urlsplit only splits on a literal '?', so a URL that encoded it kept its signing parameters in the path. - Drop comments that restate the code, docstrings that restate signatures, a guard against an input that cannot occur, and tests that cannot fail unless another test also fails. The response mock now yields chunked bodies and empties on block exit, both of which previously hid real regressions from the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018cayDdcs1iuui8Pgh5npKm
sql_caching.py had grown a log-redaction subsystem alongside the cache flow. Move the redaction, parsing and formatting helpers to sql_cache_diagnostics.py and colocate their tests, leaving sql_caching.py as the read/write flow it started as. Names crossing the new module boundary drop their leading underscore. The repeated redact-and-bound idiom becomes _redacted_snippet, which keeps the field-length bound private to the new module. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018cayDdcs1iuui8Pgh5npKm
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
2 participants
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.
@coderabbitai ignore
Closes #113.
What was wrong
upload_sql_cachereported failures using only the string produced byraise_for_status(). For presigned S3 uploads that discarded the one thing identifying the cause (the response body) while including the one thing that should never reach logs (the presigned URL, which carriesX-Amz-CredentialandX-Amz-Security-Token). Because a unique URL was interpolated into every message, no two occurrences shared a message string, so downstream error tracking saw a stream of unrelated errors instead of one issue with a rising count.The change, in one before/after
Before — actual production output, redacted:
After — the same 403, run through the new code:
The last two lines are the point:
931.4 > 900makes an expiry-driven failure self-evident, ands3_error_codeseparates the four causes that all present as a bare 403 today (AccessDenied+expired,ExpiredToken, plainAccessDenied,SignatureDoesNotMatch).What's in it
<Code>,<Message>,<Expires>,<ServerTime>from the body, plusx-amz-request-id/x-amz-id-2/Date. Only those four elements are read —<CanonicalRequest>,<StringToSign>and<AWSAccessKeyId>echo the signed query string and are never logged.<Expires>/<ServerTime>/Dateare AWS's own clock-independent answer to "did the URL run out of time", independent of this pod's accounting.raise_for_status()is gone; the status check is explicit and its failure path never builds a string containing the URL. A redaction pass strips the query string off anything URL-shaped in any text destined for a log.get_sql_cachenow returns aSqlCacheUpload(url, issued_at)NamedTuple;issued_atis taken before the webapp round-trip so the measurement is a conservative upper bound on lifetime consumed.logger.errorsites in the module take a single string literal with everything variable inextra.failed_to_serialize_cacheinstead of being conflated with upload failures, and log only the exception type — the message quotes user column names.pd.read_parquet(download_url)fetched the URL with urllib internally and raised before S3's body was ever read. The object is now fetched explicitly, so the same diagnostics are available. The parquet→pickle fallback is preserved and now downloads once instead of twice.timeout=(5, None)on both transfers. Read is left unbounded so slow-but-healthy transfers of large cache objects are unaffected; a hung connect is unambiguously a bug and its exception is swallowed like any other.Cache failures remain swallowed and can never fail the user's query.
Notes for the reviewer
Two things worth knowing, both found while verifying rather than assumed:
Removing
raise_for_status()is not sufficient on its own.requests'ConnectionError/Timeout/SSLErrorembed the URL via urllib3'sMaxRetryError, in a scheme-less, path-only form:So the redaction pattern deliberately makes the scheme optional. A
https?://-anchored pattern is a no-op on this input, which is the shape that actually occurs on network failures.urlsplit(None)does not raise — it returnsSplitResultBytes(path=b'', ...). Abytesvalue inextramakesjson.dumpsfail insidereport_error_to_webapp, which is outside anytry, so the entire error report is silently discarded. Both URL helpers now guard onisinstance(url, str).Disproven hypothesis, so it isn't re-litigated: chunked encoding is not the cause of the 403s.
requests.put(url, data=<tempfile>)sendsContent-Lengthwith noTransfer-Encoding—super_lenresolves the length from the fd.Testing
tests/unit/test_sql_caching.py: 26 tests in 31.1s → 74 in 1.4s.The old suite contained a test making a real network call to
localhost:19456whoseread_parquet/read_picklemocks were provably dead — it exited through the cache-info handler and never reached the code it claimed to test. It is now patched properly and asserts both mocks were called.Tests pin each acceptance criterion mechanically, including: no credential survives into any logged field across ~40 adversarial inputs; the message string is identical across different failure kinds; no
extrakey collides with a reservedLogRecordattribute; everyextravalue is JSON-serializable on every failure path; andbuffer.seek(0)before the pickle fallback is pinned with real pickle bytes, so removing it fails loudly rather than silently breaking every pickle-format cache read.black/isort/flake8/mypyclean. Full suite: 1016 passed, 4 skipped.Follow-ups, deliberately not in this PR
LoggerManagerre-adds handlers on everyget_logger()call. The re-entry guard atlogging.py:238only fires when the level changes, sofile_loggeraccumulates ~10WebappErrorHandlers and every toolkit error is reported to the webapp ~10 times. Invisible in CI, where the handler isn't attached. Pre-existing, but this PR ships richerextradicts so it amplifies. Wants its own issue.seconds_since_url_issued. It is logged only on failure, so there's no distribution to judge 931s against. Emitting it viaoutput_sql_metadatawould fix that, but that payload is a contract with the webapp — out of scope here.isort .also rewritesdeepnote_toolkit/streamlit_data_apps.py, which is mis-sorted onmainand unrelated. Left alone to keep this diff scoped.🤖 Generated with Claude Code
https://claude.ai/code/session_015xJczfWE4mq3ux7ZU9ZeU9