Skip to content

Commit da788bf

Browse files
committed
perf(diff): tighten diff-scan poll ceiling and make its timing attributable
A finished comparison could sit unobserved for up to 30s between polls, which is dead time on every PR job. Lower the ceiling to 10s: a multi-minute comparison costs roughly 2x the polls while cutting worst-case dead time to 10s. Diff scans now log their ID, poll count, and the wait before the final poll at INFO. Previously the ID was debug-only, so a slow comparison in a customer CI log could not be tied back to a server-side diff scan, and there was no way to tell backend comparison time apart from time the result spent ready-but-unpolled. Also document the diff-scans token scopes. A token missing them still completes the scan, silently falling back to the streaming comparison, which differs in both transport and payload (cached diff-scan responses always embed per-package license details; the streaming path requests a lean payload). Ref: CE-379
1 parent 0fc1cb3 commit da788bf

4 files changed

Lines changed: 99 additions & 2 deletions

File tree

CHANGELOG.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,17 @@
2222
smaller local fallback pattern set. Manifest results from `--sub-path` routing
2323
are reused during scan creation.
2424

25+
### Changed: scan comparison timing is easier to attribute
26+
27+
- Lowered the diff-scan poll ceiling from 30s to 10s. A finished comparison is no
28+
longer left unobserved for up to half a minute, which matters when the CLI runs
29+
inside a CI step with a per-step time budget.
30+
- Diff scans now log their ID, poll count, and the wait before the final poll, so
31+
a CI log distinguishes backend comparison time from time spent between polls.
32+
- Documented the `diff-scans:create`, `diff-scans:list` and `full-scans:list`
33+
token scopes. Without them the comparison silently falls back to the older
34+
streaming path.
35+
2536
## 2.6.4
2637

2738
### Changed: bump pinned @coana-tech/cli to 15.10.13

docs/troubleshooting.md

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,34 @@
11
# Troubleshooting
22

3+
## API token scopes for scan comparisons
4+
5+
PR/MR runs compare the new scan against the repository's head scan. That comparison
6+
first uses the diff-scans endpoints, which require an organization token with these
7+
scopes in addition to whatever the scan itself needs:
8+
9+
- `diff-scans:create`
10+
- `diff-scans:list`
11+
- `full-scans:list`
12+
13+
If the token is missing them the scan still succeeds, so this is easy to miss. The only
14+
signal is a warning, after which the CLI falls back to the older streaming comparison:
15+
16+
```
17+
Diff scan comparison failed with APIAccessDenied(Insufficient permissions), falling back to the streaming scan comparison
18+
```
19+
20+
Grant the scopes to use the diff-scans path. It polls with short, bounded requests
21+
rather than holding one connection open while the backend computes, which is what lets
22+
large comparisons survive network idle timeouts — notably Azure NAT gateways, which
23+
reap idle connections after four minutes and surface as an intermittent
24+
`ConnectionResetError`.
25+
26+
The two paths can take noticeably different amounts of time on the same repository,
27+
because cached diff-scan responses always embed per-package license details while the
28+
streaming comparison requests a lean payload. On a large dependency tree, compare the
29+
`Diff scan comparison ready in ...` timing against the `Diff Report Gathered in ...`
30+
total before assuming either path is at fault.
31+
332
## Common gotchas
433

534
- In diff scope, `--strict-blocking` uses a stricter alert set (`new + unchanged`) for blocking checks and diff-based output selection.

socketsecurity/core/__init__.py

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,8 +103,15 @@
103103
# minutes to compute. The timeout is a backstop against a diff scan that never
104104
# completes; on expiry (or any other failure of this flow) the caller falls back to the
105105
# legacy streaming comparison rather than failing the scan outright.
106+
#
107+
# The max interval is also the upper bound on how long a finished comparison sits
108+
# unnoticed between polls, which is dead time added to every PR job. Callers commonly
109+
# run this inside a CI step with a per-step time budget of a few minutes, so the cap is
110+
# kept small: a multi-minute comparison costs roughly 2x the polls of a 30s cap while
111+
# cutting the worst-case dead time from 30s to 10s. Diff scans log their poll count and
112+
# last interval on completion so this tradeoff can be re-evaluated against real timings.
106113
DIFF_SCAN_POLL_INITIAL_INTERVAL_SECONDS = 5.0
107-
DIFF_SCAN_POLL_MAX_INTERVAL_SECONDS = 30.0
114+
DIFF_SCAN_POLL_MAX_INTERVAL_SECONDS = 10.0
108115
DIFF_SCAN_POLL_BACKOFF_MULTIPLIER = 1.5
109116
DIFF_SCAN_POLL_TIMEOUT_SECONDS = 30 * 60.0
110117

@@ -1584,6 +1591,10 @@ def get_diff_scan_artifacts(
15841591
"Error creating or resolving diff scan: "
15851592
f"unexpected response: {str(response_summary)[:500]}"
15861593
)
1594+
# Logged at INFO, not debug: this is the only identifier that ties a slow or
1595+
# failed comparison in a CI log back to a server-side diff scan, and it is
1596+
# needed even when the run later falls back to the streaming comparison.
1597+
log.info(f"Diff scan created: id={diff_scan_id}")
15871598
artifacts_dict = diff_scan.get("artifacts")
15881599

15891600
# cached=true is the polling contract (202 while computing, 200 when
@@ -1595,8 +1606,14 @@ def get_diff_scan_artifacts(
15951606
# response.json() fails and the caller falls back to the legacy
15961607
# streaming comparison, which still requests the lean payload.
15971608
poll_params = {"cached": "true"}
1598-
deadline = time.monotonic() + DIFF_SCAN_POLL_TIMEOUT_SECONDS
1609+
poll_start = time.monotonic()
1610+
deadline = poll_start + DIFF_SCAN_POLL_TIMEOUT_SECONDS
15991611
interval = DIFF_SCAN_POLL_INITIAL_INTERVAL_SECONDS
1612+
# Tracked so the completion log can separate backend compute time from time the
1613+
# result spent ready-but-unpolled: the wait before the final poll bounds the
1614+
# latter, which is otherwise invisible in a CI log.
1615+
polls = 0
1616+
last_interval = 0.0
16001617
while artifacts_dict is None:
16011618
try:
16021619
response = self.sdk.diffscans.get(self.config.org_slug, diff_scan_id, params=poll_params)
@@ -1610,13 +1627,19 @@ def get_diff_scan_artifacts(
16101627
f"({type(error).__name__}), retrying in {interval:.0f}s"
16111628
)
16121629
response = {"status": "processing"}
1630+
polls += 1
16131631
if response.get("status") != "processing":
16141632
scan = response.get("diff_scan") or {}
16151633
if scan.get("artifacts") is None:
16161634
raise Exception(
16171635
f"Error fetching diff scan {diff_scan_id}: unexpected response: {str(response)[:500]}"
16181636
)
16191637
artifacts_dict = scan["artifacts"]
1638+
log.info(
1639+
"Diff scan comparison ready in "
1640+
f"{time.monotonic() - poll_start:.2f}s: id={diff_scan_id}, "
1641+
f"polls={polls}, wait_before_final_poll={last_interval:.0f}s"
1642+
)
16201643
break
16211644
if time.monotonic() >= deadline:
16221645
raise Exception(
@@ -1625,6 +1648,7 @@ def get_diff_scan_artifacts(
16251648
)
16261649
log.debug(f"Diff scan {diff_scan_id} still processing, polling again in {interval:.0f}s")
16271650
time.sleep(interval)
1651+
last_interval = interval
16281652
interval = min(interval * DIFF_SCAN_POLL_BACKOFF_MULTIPLIER, DIFF_SCAN_POLL_MAX_INTERVAL_SECONDS)
16291653

16301654
return DiffArtifacts.from_dict({

tests/core/test_diff_scan_polling.py

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,3 +120,36 @@ def test_fallback_to_streaming_diff_on_failure(core):
120120
)
121121
assert "dp3" in added
122122
assert "dp2" in removed
123+
124+
125+
def test_completion_log_reports_id_polls_and_final_wait(
126+
core, diff_scan_get_response, no_sleep, caplog, monkeypatch
127+
):
128+
"""The completion log must let a CI log separate backend compute time from the
129+
time a finished comparison sat unnoticed between polls."""
130+
import logging
131+
132+
monkeypatch.setattr(core_module, "DIFF_SCAN_POLL_INITIAL_INTERVAL_SECONDS", 4.0)
133+
monkeypatch.setattr(core_module, "DIFF_SCAN_POLL_MAX_INTERVAL_SECONDS", 6.0)
134+
processing = {"status": "processing", "id": "diff-scan-123"}
135+
core.sdk.diffscans.get.side_effect = [processing, processing, diff_scan_get_response]
136+
137+
with caplog.at_level(logging.INFO, logger="socketdev"):
138+
core.get_diff_scan_artifacts("head", "new")
139+
140+
messages = [record.message for record in caplog.records]
141+
assert any("Diff scan created: id=" in message for message in messages)
142+
ready = next(message for message in messages if "Diff scan comparison ready" in message)
143+
assert "polls=3" in ready
144+
# Waits were 4s then 6s (capped); the final poll followed the 6s wait, which is
145+
# the upper bound on how long the result was ready before being observed.
146+
assert "wait_before_final_poll=6s" in ready
147+
148+
149+
def test_max_poll_interval_bounds_dead_time_for_ci_budgets():
150+
"""A finished comparison is never left unobserved longer than the max interval."""
151+
assert core_module.DIFF_SCAN_POLL_MAX_INTERVAL_SECONDS <= 10.0
152+
assert (
153+
core_module.DIFF_SCAN_POLL_INITIAL_INTERVAL_SECONDS
154+
<= core_module.DIFF_SCAN_POLL_MAX_INTERVAL_SECONDS
155+
)

0 commit comments

Comments
 (0)