onion-time-pre-script: land the output-stability fixes on master - #79
Closed
assisted-by-ai wants to merge 3 commits into
Closed
onion-time-pre-script: land the output-stability fixes on master#79assisted-by-ai wants to merge 3 commits into
assisted-by-ai wants to merge 3 commits into
Conversation
- anondate_use created /run/sdwdate/request_anondate-set on every call.
- sdwdate's preparation loop calls this script about once per second while
Tor is unreachable, and the consumer
(sdwdate/usr/libexec/sdwdate/sdwdate-start-anondate-set-file-watcher) is a
bare inotify read loop with no limit of its own, so every request became a
full anondate-set run.
- Suppress a request when the previous one is under 60s old. Nothing
anondate-set consumes moves faster: it derives the time from the Tor
consensus (hourly, valid 3h) or the Tor certificate lifetime.
- Timestamp kept in its own file; the consumer deletes the request file once
serviced, so the request file's mtime cannot carry the state.
- Stamp written before the request file, so a death between the two leaves
the limit armed rather than unarmed.
- First request after boot is unaffected (/run is empty, no stamp).
- A stamp dated in the future means the clock moved backwards, which is
expected because anondate-set and sdwdate both set the clock. Fail open.
Regression test: onion-time-pre-script-testscript, wired into ./run-tests.
Verified 11 pass / 0 fail / 0 skip on the fix, and 2 fail on the pre-fix
code (including the request-not-recreated assertion).
Drive-by, to leave shellcheck clean on the touched file:
- exit_handler ran its "No exit code set yet. Setting to 1." string as a
command and never set exit_code (SC2288).
- output_cmd msg="$@" -> "$*" (SC2124).
- $FUNCNAME -> ${FUNCNAME[0]} (SC2128).
ci/lint-install.sh: install safe-rm, used by the shell tests in ./run-tests.
Co-Authored-By: Claude <noreply@anthropic.com>
- run-tests: move onion-time-pre-script-testscript ahead of the python suites. run-tests is errexit, and the stdisplay suite is currently red in CI, so the shell test was never reached -- wired in but not running. - onion-time-pre-script: brace all 77 SC2250 sites (R-020), drop the blank-line separator (R-042). - run-tests, ci/lint-install.sh: R-010 strict-mode block; brace $PWD. R-010 on onion-time-pre-script is knowingly left failing. It reads 18 variables it never assigns (most from the sourced tor_bootstrap_check.bsh) and relies on unset-expands-to-empty, so nounset needs its own audit and a test round on the time-sync path. The file failed R-010 the same way before this branch touched it. Regression suite still 11 pass / 0 fail / 0 skip after the rewrite. Co-Authored-By: Claude <noreply@anthropic.com>
sdwdate's preparation loop lengthens its retry wait after each failed run of this script, but drops back to the minimum whenever the output differs from the previous run. Tor's bootstrap line carries COUNT=, a retry counter incremented on every failed connection attempt, so on a Gateway with no network the output never repeated, the wait never advanced, and the loop respawned six Tor control port helpers per second indefinitely: https://forums.whonix.org/t/sys-whonix-has-extreme-cpu-usage-before-an-internet-connection/23450 QubesOS/qubes-issues#11013 Only the printed copy is redacted. ${tor_bootstrap_status} keeps the counter, so the REASON=TIMEOUT match below is unaffected, and systemcheck still shows the full line in the eleven places it displays it -- which is where this script's own log messages already tell the user to look. COUNT= is the only field that moves on every run. The consensus valid-after/valid-until timestamps only change when a new consensus arrives, and timesanitycheck emits a static message. Redacting is a denylist over a format we do not own, so sdwdate does not rely on it being complete: it also floors its retry interval, which bounds the cost regardless of what this script prints. Regression tests in dist-ai (onion-time-pre-script-tests). They pin BOTH directions -- a counter tick must not change the output, and a PROGRESS/REASON transition must still change it -- because a redaction that silenced real transitions would trade a CPU bug for a responsiveness bug. 11 pass, 0 fail, 0 skip; against the unredacted script 4 fail. Generated with assistance from Claude Code.
📝 WalkthroughWalkthroughThe helper script now rate-limits anondate requests with timestamp files, improves shell quoting and diagnostics, redacts variable Tor retry counts, and adds regression coverage. CI installs ChangesAnondate and shell reliability
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TestScript
participant anondate_use
participant RequestFiles
TestScript->>anondate_use: Invoke anondate_use
anondate_use->>RequestFiles: Read timestamp stamp
anondate_use->>RequestFiles: Write stamp before request file
anondate_use-->>TestScript: Return request status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@usr/libexec/helper-scripts/onion-time-pre-script`:
- Around line 397-411: Update the anondate request path around
anondate_request_rate_limited to acquire a shared atomic lock before checking
the rate-limit stamp. After the lock is acquired, re-check the stamp and exit
with the existing rate-limit result when it is fresh; only then create
request_anondate-set.stamp and request_anondate-set while holding the lock,
releasing it afterward.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 68eec248-81f7-412b-b26d-0d2f6df2439d
📒 Files selected for processing (4)
ci/lint-install.shonion-time-pre-script-testscriptrun-testsusr/libexec/helper-scripts/onion-time-pre-script
Comment on lines
+397
to
+411
| if anondate_request_rate_limited; then | ||
| output_cmd "anondate_use: Skipping 'anondate-set' request, the previous request is less than ${anondate_request_min_interval_seconds} seconds old, ok." | ||
| exit_code=2 | ||
| exit "${exit_code}" | ||
| fi | ||
|
|
||
| ## https://gitlab.com/apparmor/apparmor/-/wikis/AppArmorStacking#seccomp-and-no_new_privs | ||
| ## https://www.whonix.org/wiki/Dev/TimeSync#Fixing_Time_based_on_Tor_Consensus | ||
| ## /usr/libexec/sdwdate/sdwdate-start-anondate-set-file-watcher | ||
| output_cmd "anondate_use: Running 'anondate-set' (by creating file '$anondate_state_folder/request_anondate-set')..." | ||
| touch "$anondate_state_folder/request_anondate-set" | ||
| output_cmd "anondate_use: Running 'anondate-set' (by creating file '${anondate_state_folder}/request_anondate-set')..." | ||
|
|
||
| ## Stamp before request: if this script dies between the two, the next call | ||
| ## is still rate limited. The reverse order would leave the limit unarmed. | ||
| touch "${anondate_state_folder}/request_anondate-set.stamp" | ||
| touch "${anondate_state_folder}/request_anondate-set" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make the rate-limit claim atomic.
Two concurrent calls can both observe no fresh stamp at Line 397. Both calls can then create a request at Lines 410-411. This bypasses the one-request-per-60-seconds requirement and can start multiple anondate-set runs.
Use a shared atomic lock, then re-check the stamp after acquiring the lock before creating either file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@usr/libexec/helper-scripts/onion-time-pre-script` around lines 397 - 411,
Update the anondate request path around anondate_request_rate_limited to acquire
a shared atomic lock before checking the rate-limit stamp. After the lock is
acquired, re-check the stamp and exit with the existing rate-limit result when
it is fresh; only then create request_anondate-set.stamp and
request_anondate-set while holding the lock, releasing it afterward.
Author
|
Closing: this branch cherry-picked commits that already live on the |
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.
master is red on
dist-ai-tests, 4 failures:Those are the regression tests for the onion-time-pre-script busy-wait. dist-ai master carries the tests; helper-scripts master lacks the fixes, so the tests fail here rather than on the branch that introduced them -- the usual cross-repo ordering problem where the suite and the component land separately.
The bug the tests cover
sdwdate's preparation loop lengthens its retry wait after each failed run, but resets to the minimum whenever the output differs from the previous run. Tor's bootstrap line carries
COUNT=, a retry counter incremented on every failed connection attempt, so on a Gateway with no network the output never repeated, the wait never advanced, and the loop respawned six Tor control-port helpers per second indefinitely.Contents
Three commits cherry-picked unchanged from
ai(PR #78), authored by other sessions -- this PR only lands them on master ahead of that branch:dc73dc42rate limit anondate-set requeststest_unset_exit_code_becomes_one(carries theexit_handlerfix -- it ran its "No exit code set yet. Setting to 1." string as a command and never setexit_code)327bd174brace variable references, run shell tests firstda46a506redact Tor's retry counter from the outputTestBootstrapOutputStabilityfailuresOnly the printed copy is redacted --
${tor_bootstrap_status}keeps the counter, so theREASON=TIMEOUTmatch is unaffected and systemcheck still shows the full line.Note
PR #78 contains these same commits among 48 others and currently reports
mergeable_state: unstable. If #78 lands first this PR becomes redundant and should be closed unmerged; it exists so master can go green without waiting on the rest of that branch.Generated with assistance from Claude Code.
Summary by CodeRabbit
Bug Fixes
Tests