feat: auto-disable post/presign/s3 sinks on repeated errors#689
Merged
Conversation
Hard errors (4xx except 429) disable the sink immediately. Soft errors (5xx, 429, network failures) disable it after a configurable number of consecutive failures (disable_after_errors, default 3). The counter resets on any successful delivery. Raises HttpStatusError from nimutils so the status code is available as a typed field, avoiding string parsing of error messages. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…opped onHttpSinkError set cfg.enabled=false and fell through without re-raising on the hard-4xx and threshold-tripping soft branches, so nimutils publish() counted the failed delivery as a success (result += 1), never invoked onFail, and the report that tripped the disable was neither added to sinkErrors nor buffered in the report cache. Only the below-threshold soft branch re-raised, so the same failure had two outcomes depending on the failure count. Re-raise on every path of onHttpSinkError so the triggering delivery is always routed through publish()'s onFail -> sinkErrors -> report cache, exactly like a below-threshold soft error. Disabling now only stops future publishes; it no longer discards the current report. Drop the now-unreachable returns in presignSinkOut's sign-request except blocks. Extends the existing post-error disable test and its fixture (renamed to post_errors.c4m) to drive a hard 400 and a threshold-tripping soft 500 in a single chalk run, asserting each report is still buffered in the report cache (both cache assertions fail without the re-raise). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A disable_after_errors value of 0 or a negative integer previously passed validation and yielded a threshold <= 0, so the first soft error satisfied count >= threshold and immediately disabled the sink -- contradicting the "must be a positive integer" message that only fired for non-int values. - chalk.c42spec sink_config_check now validates disable_after_errors as a positive integer (split from the priority type check), failing config load with "must be a positive integer" for non-positive or non-int values. - getSinkConfigByName rejects non-positive values with the same message and restores the explanatory comments matching the timeout/truncation_amount sibling branch. - sinkErrorThreshold clamps to a minimum of 1 as a defensive floor so the disable machinery can never be tripped on the first soft error even if a non-positive threshold reaches it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
In presignSinkOut the missing-Location and relative-Location checks raised a bare ValueError between the sign-request try/except and the upload try/except, i.e. outside any try that calls onHttpSinkError. onHttpSinkError is the only code that mutates the consecutive-failure counter and sets cfg.enabled=false, so a malformed redirect escaped the disable machinery entirely: it propagated to the outer publish() loop and chalk's ioErrorHandler (which never disables an http/presign sink), leaving the sink to retry indefinitely. Every other presign failure mode (sign and upload request errors) already routes through onHttpSinkError. Wrap the Location validation in a try/except that routes failures through onHttpSinkError(hard = true): a sign response with a missing or non-absolute Location can never produce a working upload, so it disables the presign sink immediately like a 4xx, consistent with the other failure modes. Because onHttpSinkError re-raises on every path, the triggering report is still accounted as a failure and buffered in the report cache rather than dropped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The disable_after_errors (and s3 endpoint) rows widened the first/last cells so per-row pipes no longer lined up with the header and separator rows in the s3 and post markdown parameter tables. Re-pad every cell so all pipes align with the separator. Whitespace-only; rendered output and table content are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tract onHttpSinkError re-raised the in-flight exception via a bare `raise`, which only works while an exception is being handled. Neither the name, signature, nor a comment documented this precondition, so a future call from outside an except block (or a refactor separating classification from handling) would fail at runtime with a ReraiseDefect. Take the caught exception as an explicit `err: ref Exception` parameter and `raise err`, and add a doc comment stating the always-re-raise contract. Behavior is unchanged: `err.msg` reproduces the previous `reason` string byte-for-byte at every call site, and the exception is still re-raised on every path so the triggering delivery is accounted as a failure and buffered in the report cache rather than dropped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lers The hard-vs-soft classification rule `e.code in 400..499 and e.code != 429` was copied verbatim into the HttpStatusError handler of s3SinkOut, postSinkOut, and the presign sign phase, so a one-line policy change would have required three synchronized edits. Extract it into a single module-private predicate isHardHttpError and have all three call sites use it, leaving the deliberate presign divergences (redirect = hard, upload = soft) untouched. Behavior-preserving: no user-visible change. Confirmed by test_sink.py (9/9), including test_post_400_disables_sink (hard) and test_500_http_fastapi (soft). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The s3 sink shares the post/presign auto-disable machinery (hard 4xx except 429 disable immediately; soft errors disable at disable_after_errors), but the only s3 test was credential-gated and happy-path only, so the s3 hard/soft classification and disable path never ran in CI. Add error-injecting S3-style PUT routes to the test server and fold s3 coverage into the shared disable test: it now drives post and s3 sinks against 400/500 endpoints in one run, with the s3 sinks pointed at a fake S3-style endpoint so the path runs without real AWS credentials. The credential-gated happy-path test_s3 is left untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Summary
counter and disable the sink once it reaches the threshold.
disable_after_errors: <int>parameter onpost,presign, ands3sinkssets the soft-error threshold (default: 3).
isHardHttpErrorpredicate extracted so the hard-vs-soft policy is defined once;all three sink output procs reference it.
onHttpSinkErrortakes the caught exception explicitly (err: ref Exception) andre-raises it, so the triggering delivery is still recorded as a publish failure and
buffered in the report cache rather than silently dropped.
HttpStatusErroradded to nimutilsnet.nim(subtype ofValueError) so the HTTPstatus code is available as a typed field — existing
except ValueErrorblocks areunaffected.
Locationheader) now routethrough
onHttpSinkErrorinstead of escaping the disable machinery.disable_after_errorsvalidated as a positive integer at sink config load time.chalk.c42specupdated to acceptintfordisable_after_errorssink field(the catch-all validator required all unknown fields to be strings).
Makefilenimutils_versiontarget now reads directly fromchalk.nimbleso italways shows the pinned SHA regardless of interactive mode.
test_400_disables_sinkdrives post and s3 sinks against 400/500endpoints in one run (s3 via a fake endpoint — no AWS credentials needed),
asserting hard-disable, soft-disable, and report-cache buffering for each.
Test plan
make tests args="test_sink.py::test_400_disables_sink test_sink.py::test_post_http_fastapi test_sink.py::test_presign_http_fastapi -x -s"🤖 Generated with Claude Code