Report unexpected exceptions that DrDr swallows - #16
Merged
Merged
Conversation
Callers that move a path between build roots explode both paths, compare the first elements, and strip them. This helper does that once, and the next commits use it.
An archive records the root its build had when the archive was created. `archive-extract-path` dropped as many leading elements from a query as that root has, without comparing them, so a query under any other root of the same length resolved to an entry at the wrong level. A build that has aged out to the extra build directory lives under such a root. Compare the prefix instead, and add a `#:base` argument so that a caller that knows where the build lives now can say so. cache.rkt passes the build's current directory. Primary builds behave as before. Archived builds still fail, as they do today, because `path->revision` cannot parse their paths; the next commit fixes that.
`path->revision` indexed into an exploded path at the length of the primary build directory. Builds that have aged out live under the extra build directory, which is one element shorter in production, so the index landed on "logs" and `string->number` returned #f, breaking the function's contract. `cached-directory-exists?` caught that exception and reported the build as absent, so `find-previous-rev` walked backwards one revision at a time, and every other archive lookup during a render failed the same way. A page for an archived revision spent about 42 seconds of CPU to conclude "Not Found"; with this change it renders in about 0.2 seconds. Because every file page links to "next change" for its revision, those pages also trapped crawlers: for an archived revision the link redirected back to the page itself. Match a root prefix against either build directory instead.
archive-repair looked up a build's current directory inside its archive, but the archive names its contents by the directory the build had when the archive was created. For a build that has moved to the extra build directory the two differ, and the repair failed. Pass the current directory as `#:base`.
render.rkt recognized a cache miss by matching the text of an error raised in cache.rkt, so rewording that error would silently break the handlers. Raise `exn:fail:not-cached` for misses in cache.rkt, archive.rkt and `path->revision`, and test for that type instead. The handlers that matched "No cache available" now also catch a miss that `read-cache` raises inside the call they guard. Such a miss used to escape as an internal error; now the page reports the file or directory as not found.
The cache lookups caught every `exn:fail?` and returned "not cached", so the contract violation that `path->revision` raised for every archived build looked like an ordinary miss for months. Add `swallow` to notify.rkt: it returns a fallback as before, but it logs a `drdr` warning for any exception its caller has not declared expected. The renderer's service unit sets PLTSTDERR=warning, so these warnings reach its journal. Use it in the cache lookups and in `archive-directory-exists?`, where a missing file or a missing archive entry stays quiet. `read-cache*` now also reports a corrupt cache file, which `file->value` rejects.
Apply `swallow` where DrDr substitutes a default for a failed lookup: analyze's committer, diff, changed? and commit-author fallbacks, gather-logs, `safely-delete-directory`, `check-xvfb-info`, and render's build timestamps and previous-revision comparison. A failure in `check-xvfb-info`, for example, would silently run GUI tests without Xvfb. The committer lookup now calls `read-cache` rather than `read-cache*`. The latter turns a miss into #f, which then breaks `scm-commit-author`'s contract and would be reported as a bug instead of as the missing commit message it is.
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.
This PR is stacked on #14 and should land after it; only the last three commits are new. Without #14, every archived-build lookup would log a warning.
The
path->revisionbug fixed in #14 went unnoticed for months because the cache lookups turned everyexn:fail?into "not cached". This PR makes such failures visible:exn:fail:not-cached. render.rkt then tests for that type instead of matching error text.swallowto notify.rkt. It returns the same fallback as before, and it logs adrdrwarning for any exception that its caller has not declared expected. The cache lookups andarchive-directory-exists?now use it. The renderer's unit sets PLTSTDERR=warning, so these warnings reach the journal.swallowto the other places that substitute a default for a failed lookup: analyze, gather-logs,safely-delete-directory,check-xvfb-info, and render's timestamps.On hurin, with #14 underneath, the patched renderer serves the same pages as the deployed one and logged no warnings.