A mirror the engine gave up on was announced as a success - #162
Merged
Conversation
hts_main2() returns 0 for nearly every abort, so the finished pane read a run that died as one that finished: a full disk, a full link table or a session rolled back for want of a connection all arrived at "Success! (N errors)", with the errors count as the only hint that anything was wrong. The engine knows, and now says so. abortReason() asks hts_is_exiting() and returns hts_errmsg(), so the pane reports the abort and its reason ahead of any success. The text comes from the engine into a pane that renders HTML, so it is escaped. This reads the exit_xh the engine already sets at our pin. The full-disk half reaches it through httrack 5b5e3d25, which is upstream of the pinned 3.49.23, so that case starts working when the pin next moves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Review of the first commit found two defects in it. The abort branch sat ahead of the interrupted one, but exit_xh is not a sign that the engine gave up on its own: a stop early enough that nothing was saved is rolled back for want of data (htscore.c:2088, on a guard that never consults state.stop), and a forced stop refuses the loop callback (htscore.c:951/1072). Both of the commonest ways a user ends a crawl were about to be announced as aborts nobody asked for, undoing #158. The reason string could never arrive. _hts_errmsg has one writer, HTS_PANIC_PRINTF, whose call sites all sit in htscoremain.c ahead of the mirror and return -1, so main() returning 0 and a non-empty hts_errmsg() are disjoint. Every abort would have read "mirror aborted", and the escaping around it guarded text that cannot exist. So abortCode() hands back exit_xh itself and the three causes carry three messages, the full link table is dropped from the claim (htsparse.c returns -1 without touching exit_xh), and no engine string crosses JNI. Choosing the outcome moves to MirrorOutcome, which imports no android.* and is table-tested; both defects above fail it under mutation. between() moves to TestSources, where two tests had already inlined it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Re-review of the rewrite found four things, two of them mine to answer for. MirrorOutcome.of() took the return code, so its arguments were evaluated before it could test them, and lastStats was dereferenced on the code != 0 path where master never touched it. lastStats is nullable, so an OOM in the loop callback turned "Error (code N)" into a NullPointerException and skipped buildTopIndex(). The return code goes back to the caller, where master had it, and of() now takes the stats object: boolean, int and HTTrackStats share no type, so an argument swap is a compile error rather than something a test has to notice. It could not notice: passing of(abortCode(), interrupted, code, ...) compiled and satisfied both wiring assertions. Two mutants survived the table. An unmapped abort code fell through to success, which is the bug this exists to kill, and the exact-text assertion on the C body failed under a harmless (jint) cast while missing nothing real. Both are covered now, and the abort messages are pinned to their causes, since swapping them was invisible. ABORTED_IO becomes ABORTED_FATAL: check_fatal_io_errno() also fires on EMFILE, EROFS and EDQUOT, and htscore.c:4020 sets the same -1 when the link table cannot record a link, so "most likely a full disk" named one of four causes. ABORTED becomes ABORTED_OTHER, which no longer reads as the parent of the two named ones, and its comment says plainly that no path is known to reach it. The rollback wording no longer promises a previous mirror a first crawl never had. The licence header on the new file was the wrong boilerplate, GPLv2 where the tree says v3; the file's closest peers carry no header at all, so it does not either. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
"No path is known to reach this" on ABORTED_OTHER was wrong: abortCode()'s own contract documents 1 as a callback refusing to continue, and the table test drives it. The switch had no default, so a constant added later would have left the message null and shipped the word "null" to the pane. Nothing would have caught it: the build sets no -Xlint, no Error Prone, and lint does not abort. It throws instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.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.
main()returns 0 for nearly every abort, so the finished pane could not tell a run that died from one that finished. A full disk, or a session the engine rolled back because nothing arrived, both reached "Success! (N errors)".abortCode()hands back the engine'sexit_xh, andMirrorOutcomedecides the wording from it. A stop the user asked for is taken first, because the engine sets that flag on its own for the two commonest ways a crawl ends early. A stop with nothing saved yet is rolled back for want of data, and a forced stop refuses the loop callback. Each remaining cause gets its own message. No engine string crosses JNI:_hts_errmsgis only written while the options are parsed, before the mirror starts.Two limits. A full disk is one of four causes behind the same
-1, alongsideEMFILE,EROFSand a link table that cannot record a link. The message names all four rather than guessing. And a disk that fills mid-crawl only sets the flag from httrack5b5e3d25, which is upstream of the pinned 3.49.23, so that case starts working when the pin moves.MirrorOutcomeimports noandroid.*, so a table test pins the branch order and the unmapped-code path instead of a grep over the source.