bneradt commented on PR #13545: URL: https://github.com/apache/trafficserver/pull/13545#issuecomment-5763813800
@bryancall Thanks for the detailed assertion-by-assertion review. I rebased onto master `7741a2b237`, converted the newly added cache tests, and retained both the busy and quiet Lua shutdown scenarios added upstream. The review changes are: 1. Native ATS teardown now fails on an unexpected exit, including status 0 and signal exits. Explicit stop/kill and validated one-shot waits remain supported. A missing diagnostic destination fails too; renamed logs and stdout/stderr are checked at their configured destination. Regression tests cover these cases. 2. Background-fetch and the affected abuse-shield negative assertions now read the complete output after stopping ATS. Reload polling requires the reload to leave `in_progress` before accepting its assertions. 3. Replay `file_checks` now implements `excludes`, rejects unknown keys (including nested match keys), and enforces `line_count_min` after its polling deadline. Regression tests exercise rejected assertions and invalid manifests. 4. Every abuse-shield instance now verifies the startup rule count from its configured rules before sending traffic. 5–8. Embedded replays use separate child directories under the native scenario, including distinct directories for repeated calls and variants. Collision diagnostics are printed before raising so they remain visible with xdist. A controller-held exclusive sandbox-root lock rejects concurrent sessions before they can reset the port counter or delete artifacts; the diagnostic tells callers to choose a different `--sandbox`. On point 7, I deliberately kept the full readable test names requested for this branch instead of restoring truncation. Both JSON-RPC and HTTP Unix sockets use short paths independently of those names; a regression test binds both actual sockets under long sandbox/process names. Unrepresentable filesystem components now fail explicitly. This does not impose a blanket limit for arbitrary future path-limited artifacts; those would need the same short-path treatment. 9–11. The record-triggered reload uses bounded polling, with a fresh trigger marker so an earlier reload cannot satisfy it. `rpc-greet` rejects all returned errors. The abuse-shield checks again require `for IP=`, and the shared-log-interval check requires exactly `127.0.0.1`. 12. I retained the slice diagnostic alternation: it is already present in the original AuTest, together with the timing explanation ([original lines 295–304](https://github.com/apache/trafficserver/blob/7741a2b237/tests/gold_tests/pluginTest/slice/slice_stale_generation.test.py#L295-L304)). An abort can arrive before or after the response header. The converted test still requires client failure, failed transaction identity, and the specific block/ETag mismatch diagnostics. I brought the explanatory comment across as well. 13. Runroot verification no longer recursively changes permissions. It verifies the copied tree as the initializing user with `--with-user`, leaving copied file modes untouched, and adds a deliberately unwritable log to require verification to fail. 14. Only the locked session controller initializes the port counter. Missing/corrupt/exhausted state produces a path-bearing error; only `EADDRINUSE` retries, while other bind errors report the address and OS error. Regression tests cover these cases. 15. The two ineffective native `disable_log_checks` flags were removed. I left the inherited `idle_connections.py` behavior unchanged in this conversion. The connection-rate scenario additionally requires the rule-match log, blocked-action metric, and rejected-connection metric, so a single successful connection cannot by itself make that scenario pass. The helper's partial-success reporting remains a separate follow-up. Validation on the rebased branch: - Build/install and the full CMake formatting target succeeded. - All 1,116 CTest cases passed. - All 103 Uranium framework tests passed. - The final eight-worker selection covering cache tests and review-affected scenarios finished with **143 passed, 4 skipped**. This includes the new upstream cache replay, both Lua shutdown paths, renamed/stdout/stderr diagnostics, reload tests, runroot verification, and abuse-shield scenarios. - An earlier full eight-worker run finished with 1,260 passed, 39 skipped, and one teardown error in the renamed/redirected-log scenario. That exposed the diagnostic-destination edge case described above; the fix is covered by both regression tests and the successful final scenario run. I have not rerun the entire suite after the final rebase. There are no inline review threads to resolve; points 7 and 12 above are explicit responses rather than changes to the requested naming policy or the original slice contract. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
