fix(swarm-coordination-registry): [#2226] recompute inactivity cutoff per tick - #2252
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
This PR fixes the swarm coordination registry activity metrics job so the “inactive peer” cutoff is recomputed on every update tick (instead of using a stale startup cutoff), and adds a regression test using a stopped clock to validate the behavior.
Changes:
- Pass
max_peer_timeoutinto the registry metrics job and compute the inactivity cutoff inside each tick. - Remove the bootstrap-layer helper that computed a single cutoff timestamp at startup.
- Add a stopped-clock regression test to ensure peers become inactive after time advances post-start.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/bootstrap/jobs/activity_metrics_updater.rs | Adjusts bootstrap wiring to pass max_peer_timeout instead of a precomputed cutoff. |
| packages/swarm-coordination-registry/src/statistics/activity_metrics_updater.rs | Recomputes inactivity cutoff per tick and adds a regression test using a stopped clock. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2252 +/- ##
===========================================
+ Coverage 85.49% 85.79% +0.29%
===========================================
Files 353 353
Lines 31333 31418 +85
Branches 31333 31418 +85
===========================================
+ Hits 26789 26954 +165
+ Misses 4165 4087 -78
+ Partials 379 377 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
The bootstrap job file no longer imports std::future::Future while still using -> impl Future<...>, which will fail compilation.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
e56330f to
48ce314
Compare
…regression coverage
…ivity boundary tests Introduce JobWithOnePeerAnnouncedAtStartup so both boundary tests only state the peer timeout, the elapsed domain time, and the expected inactive peers count. The fixture owns runner startup, the announced peer, the update tick, gauge reading, and cancellation on drop.
…statistics collaboration level The before/after timeout tests exercise the job together with the real Registry and Repository, so they now live in statistics::tests as collaboration tests. activity_metrics_updater::tests keeps only the job lifecycle unit tests. Missing tests identified while classifying are noted in the owning modules and tracked by the package coverage review (torrust#1347).
… computation Move the cutoff derivation out of update_activity_metrics into inactivity_cutoff(now, max_peer_timeout) so the module's core decision is directly unit-testable. Pins three cases: cutoff is the timeout before now, a zero timeout yields now, and a now earlier than the timeout falls back to the epoch so nothing is reported inactive. No behaviour change.
… decision function pattern Lessons from the torrust#2226 test refactoring, made repository-owned instead of agent memory: - write-unit-test skill: inventory what a module decides vs. forwards before writing tests, place each test at the lowest level that can observe what it asserts, and split reorganisation from refactoring commits. Add a note that async jobs under Tokio paused time involve two independent clocks. - New catalog entry: extract a pure decision function when a module's own decision is only observable through a collaborator, and move end-to-end tests to the parent module as collaboration tests. Related to torrust#1347.
…n and make boundary failures self-explanatory Mutation checks against the torrust#2226 fix showed the before/after pair catches a cutoff captured at job start but not one cached after the first update, because each test fires a single tick. Add a two-tick test on the same job and peer that fails for either variant. Assertion messages now state the gauge, the elapsed time, and the timeout, so a failure reads as a diagnosis instead of a bare left/right pair.
…troduced bug Add 'Prove a Regression Test Guards the Bug' to the write-unit-test skill: mutate the fix in the working tree and confirm the test fails, observe two iterations of the same instance for stale-state defects, and put the causal facts in assertion messages so a failure reads as a diagnosis. Two checklist items enforce it for bug fixes. Learned while verifying torrust#2226: the before/after pair caught the original startup-captured cutoff but not a cutoff cached after the first tick.
… to ride along in PRs Two maintainer decisions agents kept re-asking about during torrust#2226: - Never squash or rewrite branch history to hide superseded steps; the mistake-and-correction sequence is training material. - Minor skill/doc/process improvements learned during the work may ride along in the same PR as separate commits; separate PRs for them is unnecessary bureaucracy.
48ce314 to
72ec687
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new paused-time collaboration tests and the updated write-unit-test guidance assume a current-thread Tokio runtime for thread-local clock::Stopped, but the tests/docs don’t consistently enforce or state that, risking flaky/incorrect results.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
packages/swarm-coordination-registry/src/statistics/mod.rs:238
- Same determinism issue as the earlier collaboration test: because the job is
tokio::spawned andclock::Stopped::local_setis thread-local, this test should force a single-thread Tokio runtime to ensure the spawned job observes the same stopped-clock timeline.
#[tokio::test(start_paused = true)]
packages/swarm-coordination-registry/src/statistics/mod.rs:257
- Same determinism issue as the other collaboration tests in this module: without forcing
flavor = "current_thread", thetokio::spawned job can run on a different worker thread and miss the test’sclock::Stopped::local_setupdates, making the two-tick regression guard flaky.
#[tokio::test(start_paused = true)]
- Files reviewed: 17/17 changed files
- Comments generated: 2
- Review effort level: Lite
|
ACK e35a1a6 |
Summary
The activity metrics updater now recomputes the peer inactivity cutoff before every update tick
instead of reusing a timestamp captured at job startup.
Problem
run_jobcomputednow - max_peer_timeoutonce and passed that absolute timestamp to everytick. Any peer announced after the tracker started therefore had
updated > cutofffor the restof the process lifetime and was never reported inactive: the
swarm_coordination_registry_peers_inactive_totaland..._torrents_inactive_totalgaugesstayed at 0. Peer cleanup has its own job and was unaffected; only the metrics were wrong.
Changes
run_jobtakesmax_peer_timeout(the configured duration) instead of a precomputed cutoff;bootstrap wiring updated accordingly.
inactivity_cutoff(now, max_peer_timeout)and called with
CurrentClock::now()on every tick.Peer::updatedand the updater clarify that the cutoff is an absolutetimestamp; the naming question around
DurationSinceUnixEpochis parked as a follow-up draftin the issue folder.
Tests
activity_metrics_updater::tests::inactivity_cutoff: three pure unit tests (timeout beforenow; zero timeout yields now; now earlier than timeout falls back to the epoch so nothing is
reported inactive).
statistics::tests: collaboration tests running the job against a realRegistryandRepositorythrough a state-named fixture (JobWithOnePeerAnnouncedAtStartup), with Tokiopaused time driving the interval and
clock::Stoppeddriving domain time:inactive on the second.
fails the after-timeout test; caching the cutoff after the first tick is caught only by the
two-tick test. Production code was mutated in the working tree and restored, never committed.
tracked by Overhaul: Packages Testing #1347.
Documentation riding along
Lessons from this fix, made repository-owned:
write-unit-testskill: inventory what a module decides vs. forwards and choose the testlevel; two independent clocks in async job tests; prove a regression test fails against the
reintroduced bug.
AGENTS.md: keep intermediate commits; minor process fixes may ride along in the PR thatproduced them.
Validation
cargo test -p torrust-tracker-swarm-coordination-registry --lib statistics: 30 passed.Pre-commit (8 checks) and pre-push (nightly fmt, nightly check, nightly doc, full stable test
suite) pass on the rebased branch. Runtime verification against a fixed build is recorded in the
issue folder (
manual-verification-evidence.md).Closes #2226
Related to #1347