feat(deployment): require multi-probe agreement before behavioural enforcement - #3984
Conversation
|
Warning Review limit reached
This review includes 18 billable files and costs up to $4.50.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 57 seconds for your next included review. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: akash-network/console/.coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (18)
Comment |
9cbcd03 to
16bd935
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether "behavioural"-confirmed deployments get the same fast-path skip that "hard" detections get on a racing re-enqueued probe job (probe-trial-deployment.handler.ts:77 only matches verdict: "hard"). They don't, but the gap is narrow: once enforcement completes it sets wallet.abuseLockedAt, which the earlier check at line 72 already skips for any verdict, so the effect is at most one extra live probe during enforcement's async window, not an incorrect-enforcement bug.
Extended reasoning...
I independently verified the ruled-out candidate about the missing fast-path skip for "behavioural" detections in apps/api/src/workload-abuse/services/probe-trial-deployment/probe-trial-deployment.handler.ts (the findOneBy({ walletId, dseq, verdict: "hard" }) check at line 77 does not include "behavioural"). Tracing the flow, wallet.abuseLockedAt is set once enforcement (wipe) actually completes via enforce-trial-abuse.handler.ts, and the earlier wallet-level check at line 72 (if (wallet.abuseLockedAt) return) already short-circuits any subsequent probe job regardless of verdict type. So the missing fast-path only matters during the narrow async window between a "behavioural" detection being recorded/enforcement being enqueued and the wallet actually getting locked — worst case is one extra live provider probe, not an incorrect or duplicate enforcement action. This is consistent with the harness having ruled it out as lower severity. Given the two CONFIRMED findings (stuck "enforcing" state / missing retry handling for the new "behavioural" verdict in the repository's stalled-enforcement queries, and the silent misconfiguration risk where tightening probe cadence env vars can make the evidence lookback window fall below the minimum agreement window) are substantive correctness issues in new security/billing-enforcement logic, not simple nits, I am not approving this PR. Since those findings are already being posted as inline comments, my body avoids restating them and only adds the additional, independently-verified check that was ruled out.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
apps/api/src/workload-abuse/repositories/workload-abuse-detection/workload-abuse-detection.repository.ts— Behavioural detections that fail or stall enforcement are never retried, and successfully-wiped ones stay stuck at action "enforcing" forever. This diff adds the "behavioural" verdict and updates findRecentDetectedTargets (line 29) to include it, but leaves the sibling queries findStalledEnforcements (line 42) and markWalletEnforced (line 57) filtered to eq(verdict, "hard") only, so they silently ignore the new verdict. Fix: update both to match verdict in ("hard","behavioural") (or drop the verdict filter and match on action) so the stall-recovery sweep and enforcement-completion bookkeeping cover every verdict that can reach the enforcing/enforcement_failed states, covering both sites.Extended reasoning...
probe-trial-deployment.handler.ts:#enforceOnRepeatedShape creates a detection with verdict "behavioural" and calls #enforce, which enqueues EnforceTrialAbuse. trial-abuse-enforcement.service.ts:enforce() sets action "enforcing" on that detection before wiping, regardless of verdict. If the wipe throws (chain error, timeout), action becomes "enforcement_failed"; the comment in that file says 'findStalledEnforcements re-queues a detection left in enforcing', but that method filters eq(this.table.verdict, "hard") so a behavioural-verdict row is never picked up and never retried - the wallet stays trialing and unwiped indefinitely. Even on success, markWalletEnforced only updates verdict "hard" rows to action "enforced", so a wiped behavioural detection's action column is stuck at "enforcing" permanently, corrupting the audit trail operators rely on for this verdict.
Verification: normal. This diff adds the "behavioural" verdict and enforces on it, but two sibling queries in workload-abuse-detection.repository.ts still filter to hard only, while findRecentDetectedTargets (line 31) was updated to inArray(verdict, ["hard","behavioural"]) — the inconsistency shows the parity was intended but missed. Primary issue (findStalledEnforcements, line 42):… | normal. This diff…
16bd935 to
082fb14
Compare
082fb14 to
c985b25
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked two related concerns: whether a confirmed "behavioural" detection keeps triggering a full live re-probe on every resurrected job (unlike "hard", which short-circuits), and whether a later probe run could silently un-confirm one. Neither regresses correctness: enforce mode locks the wallet (abuseLockedAt) before either path matters, and in detect mode the existing "behavioural" detection row is never mutated or removed, so the only real cost is redundant re-probing while in detect mode.
Extended reasoning...
This run's confirmed finding (handler.ts:217/#recordDetection) is real: when #enforceOnRepeatedShape creates a "behavioural" detection, it stores report.signals/report.excerpt from the single triggering probe rather than the multi-probe evidence agreement.ts actually scored — verified by tracing toVerdict/scanForSignals in trial-workload-probe.service.ts, which confirms signals is empty whenever verdict is "clean", the exact case that reaches this code path (handle() only calls #enforceOnRepeatedShape when verdict !== "hard"). That leaves operators in detect mode with no evidence trail for the pattern that actually fired. I additionally traced the two candidate issues surfaced by the sub-agents (re-probing of confirmed behavioural detections at line 77, and possible un-confirmation at line 207) through EnforceTrialAbuseHandler and TrialWorkloadProbeJobService and confirmed they are not correctness bugs given how wallet-lock and detection persistence interact, so a short informational note beyond the inline finding is warranted rather than staying fully silent.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
apps/api/src/workload-abuse/services/probe-trial-deployment/probe-trial-deployment.handler.ts— Deployments already confirmed via a 'behavioural' verdict lose that protection and get fully re-probed (live shell exec) whenever a new lease event fires for the same dseq, unlike 'hard' verdicts. handle() line 77 only looks up verdict:'hard' before calling probeService.probe(); a 'behavioural' detection is invisible to that check, so any later TrialDeploymentLeaseCreated for the same dseq (a normal re-lease/re-bid on the deployment) calls scheduleInitial with attempt 1 and probing resumes from scratch. In the default WORKLOAD_ABUSE_ENFORCEMENT_MODE='detect', the wallet is never locked, so this recurs on every re-lease indefinitely. …Extended reasoning...
…Fix: include 'behavioural' in the existingDetection lookup (query both verdicts) so any confirmed detection short-circuits before the live probe runs, matching the guarantee 'hard' already gets.
handle() at line 77 runs
findOneBy({ walletId, dseq, verdict: "hard" })before callingthis.probeService.probe(...). A deployment previously confirmed only has a row with verdict:'behavioural' (created at handler.ts:207-218), so this lookup returns null and the function falls through to a live probe. TrialDeploymentLeaseCreatedHandler (trial-deployment-lease-created.handler.ts:100/123) callsprobeJobService.scheduleInitialon every TrialDeploymentLeaseCreated event, including a re-lease of the same dseq (managed-signer.service.ts:154-160 publishes it whenever MsgCreateLease lands, isFirstLease only toggles a notification flag, not whether the event fires). scheduleInitial enqueues attempt=1 with the same singletonKey, so once the prior job chain stopped self-rescheduling after being confirmed, a fresh job runs. Inside it, wallet.abuseLockedAt is unset in 'detect' mode…Verification: nit. The mechanism is real and reachable. handle() line 77
findOneBy({ walletId, dseq, verdict: "hard" })is the ALREADY_DETECTED skip; it only matcheshard, so a deployment confirmed only with abehaviouraldetection (created at handler.ts:209-218) is not short-circuited and falls through to the live probe at line 91. Thebehaviourallookup exists only inside#enforceOnRepeatedShape…
c985b25 to
f47591c
Compare
f47591c to
b4feff3
Compare
There was a problem hiding this comment.
I reviewed the latest commits and found no new bugs. Since this change gates wallet-wipe enforcement and has already needed several rounds of fixes, a human look is still worthwhile before merge.
What was reviewed:
- Confirmed the two previously flagged issues are genuinely fixed in code: the evidence lookback window now takes the max of the probe-cadence span and
WORKLOAD_ABUSE_BEHAVIOURAL_MIN_WINDOW_MINUTES(2f32537), and the storedevidenceExcerpton a behavioural detection now carries the agreeing service/streak/span instead of the triggering probe's own excerpt (b4feff3). - Checked whether a user who fixes a flagged manifest could still get wiped on stale evidence — ruled out, since
measureServicescans newest-first and the streak breaks as soon as the most recent probe after the fix shows no candidate finding. - The
agreement.ts:52thread (null vs. empty findings when an evidence write fails) is marked resolved, but that code path is unchanged from the earlier flagged version.
Extended reasoning...
Overview
This PR (on top of two prior stacked PRs) adds a "behavioural agreement" signal to the workload-abuse trial-deployment probe pipeline: a pure findBehaviouralAgreement module scans recent per-service probe evidence for a consecutive streak of "candidate" behavioural findings that both meets a minimum probe count and spans a minimum time window, then ProbeTrialDeploymentHandler#enforceOnRepeatedShape uses that to create/re-enforce a new "behavioural" verdict alongside the existing hard/soft/proxy string-match path. It adds a Postgres enum value + migration, three new env vars gating/tuning the feature (all conservative-by-default), and updates repository/instrumentation/job-service call sites plus specs and integration tests. This is the sixth review of this PR by this bug-hunting system; three commits (32d1a9f, 2f32537, b4feff3) landed since the last review.
Security risks
The change can ultimately trigger a wallet wipe (via the pre-existing #enforce/EnforceTrialAbuse path), so correctness of the agreement/streak/window logic is security-relevant even though enforcement defaults to off and detect-mode gating is unchanged. I re-verified two of my own three prior findings against the new commits rather than trusting the GitHub thread status: 2f32537 directly fixes the lookback-window mismatch I flagged (now Math.max(probeSpanMinutes, minWindowMinutes)), and b4feff3 directly fixes the empty-evidence-on-clean-verdict issue I flagged (now toShapeExcerpt records service/streak/span). Both fixes match the reported bugs precisely and are covered by new/updated specs. My third finding (agreement.ts:52, about a null vs. legitimately-empty behaviouralFindings value after a failed DB write) is marked resolved on GitHub but the code at that line is byte-for-byte unchanged, so the underlying ambiguity still exists — it's a narrow, "optional"-severity edge case rooted in a different file (probe-evidence.service.ts) not touched by this diff, so I'm not re-raising it as a new finding, just noting the self-resolution doesn't reflect a code change. I also independently checked a manifest-replacement race (a user who fixes their flagged manifest getting the wallet wiped on stale evidence) and ruled it out: the streak scan is newest-first and breaks immediately at the first post-fix probe showing no candidate finding, so a corrected manifest stops accumulating before any enforcement.
Level of scrutiny
Warrants continued human attention: it affects an enforcement path with real user impact (wallet wipe), has already gone through multiple correction rounds within this same PR, and two of the three previously identified defects required dedicated follow-up commits to fix (not just test tweaks), suggesting the design still has some subtlety operators/reviewers should sanity-check (e.g., the interaction between the three new env vars). No CHANGES_REQUESTED review or unaddressed third-party objection is visible in the timeline, but the coderabbitai review content is withheld from me, so I can't confirm it's satisfied.
Other factors
Test coverage is solid for the new logic (dedicated agreement.spec.ts covering streak/skip/window/per-service selection, handler specs for agreement/one-probe-short/too-young/flag-off, and integration coverage for confirmed and one-short cases), and the diff follows the repo's clean-code/JSDoc and logger conventions well. Given no new bugs were found this run and real progress was made addressing prior findings, I'm choosing to defer with a short acknowledgment rather than stay fully silent, since confirming that self-resolved threads correspond (or don't) to actual code changes is information a human reviewer would otherwise have to re-derive themselves.
b4feff3 to
eda61d5
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3984 +/- ##
==========================================
- Coverage 84.16% 83.87% -0.30%
==========================================
Files 1318 1219 -99
Lines 36675 34105 -2570
Branches 8820 8308 -512
==========================================
- Hits 30866 28604 -2262
+ Misses 5129 4845 -284
+ Partials 680 656 -24
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
…forcement A repeated shape can now stand on its own: when consecutive probes of one service see it and the streak spans real time rather than minutes, the probe records a detection and hands the deployment to the wipe that already exists. A probe that collected nothing steps aside rather than resetting the streak. Those detections carry a verdict value of their own so the audit trail keeps them apart from a string match, and the deployment stops being re-probed once one lands. The flag is off, so nothing changes until it is set.
…reachable Agreement needs a streak that spans a configured window, but the evidence query only looked back as far as the probe cadence allows. Shorten the cadence and the oldest row it can return is younger than the span it has to prove, so agreement never lands however long the shape holds. The lookback now covers whichever of the two is longer. Agreement also reads the status of the shell that collected each row, matching where the signals are now scored.
The probe that tips a deployment over can itself be a clean one, so the row was being written with an empty signal list and a benign excerpt. It now carries the service, the number of agreeing probes and the span they cover, which is what a reviewer needs while enforcement is still off.
…r the window The mutation gate flagged the lookback math, the ordering of a history that arrives unsorted, the window boundary and the service the report names as untested. Each now has a case that fails when the behaviour changes. A probe that recorded nothing is handled where it happens instead of standing in as an empty finding list, and reading the first service of a group no longer guards against an empty one, since a group only exists because a row put it there.
d10ab06 to
cfddc25
Compare
A declared but empty environment variable never reaches a Zod default, so a blank value for any of the three enforcement settings failed config resolution for the whole module at boot. They now go through the same blank-to-undefined preprocessor their siblings already use.
…service A lease being reclaimed stays live alongside the one replacing it, so a single run can write two rows for the same service. Counting those rows as one series let a streak reach the required length in fewer runs than the setting asks for, which is the one direction that matters on a path that wipes an account. The series key now carries the provider, so each observation point counts once and a migration restarts the count instead of compounding it. The replay groups the same way so its report still predicts what the gate does.
1 participant
Why
Ref CON-979
Stacked on #3983, which is stacked on #3982. Review those first.
The signals from #3983 only record. This is the part that lets a repeated shape act on its own, and the guard that keeps one probe from being enough: the same shape has to hold across consecutive probes of a service and the streak has to span real time, not the few minutes two back-to-back probes take.
What