Skip to content

Commit 2535fba

Browse files
author
Conduction Release Bot
committed
feat(advisories): correlate in the background job and serve a stored snapshot
Closes the scope question left open on #160. The endpoint keeps covering EVERY enabled app and pays for that in staleness instead of in a request that cannot return. WHY. Correlation makes two external calls per app (listAdvisories + listVersions). On an 88-app instance that is ~176 sequential external calls, which GET /api/advisories used to do inline. Measured on a live instance it did not answer within 120s, twice, and because it was dispatched first and held the PHP session lock, the sibling /api/pins request never ran at all — so pin badges silently never rendered and nothing reported why. The alternative considered was narrowing correlation to apps with an explicit source binding: exactly 1 of the 88 here, so 176 calls become 2. That was rejected because it makes a SECURITY feature quietly stop looking at 87 apps with nothing on screen saying so. WHAT CHANGES. - AdvisoryResultStore persists the snapshot plus the time the sweep completed. A snapshot that fails to encode leaves the previous one intact: a stale answer whose age is shown beats no answer. - AdvisoryRefreshJob (already registered, already 6-hourly) now stores what it sweeps, persisting BEFORE notifying because notification is the more failure-prone half. It also warns when some apps could not be correlated. - GET /api/advisories reads the snapshot and returns `checkedAt`. - App.vue renders the age. "Swept and found nothing", "never swept because cron has not run" and "the fetch failed" otherwise render as an identical empty badge set — an absence that reads as reassurance. THE BUDGET INTERACTION, which is the subtle part. #162 gave correlateAll a 5s ceiling sized for a request someone is waiting on. That default applied to the background job too, so the 6-hourly sweep — and the notifications derived from it — were already being clipped to the first few apps. The budget is now a parameter: the job passes 600s. A regression test asserts the job passes something larger than the request-path default, and was confirmed to FAIL when the job is reverted to the bare call. PRE-EXISTING DEBT FIXED IN PASSING. satisfiesClause fed version_compare an operator psalm could not prove was in its closed set, so the declared bool return was really bool|null; it now maps through the literal set. Also drops a redundant array_values. Those four entries leave psalm-baseline.xml, which is what surfaced them: fixing them turned the baseline stale. Verification: 505 unit tests, 1020 assertions, 0 failures outside tests/unit/Command (19 errors there are a missing symfony/console in the local vendor copy, not code). psalm reports no issue in any touched file. Frontend builds; the new strings were confirmed present in the built bundle rather than assumed.
1 parent 808419a commit 2535fba

10 files changed

Lines changed: 632 additions & 40 deletions

File tree

‎lib/BackgroundJob/AdvisoryRefreshJob.php‎

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
namespace OCA\AppVersions\BackgroundJob;
1414

1515
use OCA\AppVersions\Service\Advisory\AdvisoryNotifier;
16+
use OCA\AppVersions\Service\Advisory\AdvisoryResultStore;
1617
use OCA\AppVersions\Service\Advisory\AdvisoryService;
1718
use OCP\AppFramework\Utility\ITimeFactory;
1819
use OCP\BackgroundJob\TimedJob;
@@ -31,10 +32,25 @@ class AdvisoryRefreshJob extends TimedJob {
3132
/** Re-resolve advisories every 6 hours. */
3233
private const INTERVAL_SECONDS = 6 * 60 * 60;
3334

35+
/**
36+
* Wall-clock ceiling for the sweep, in seconds.
37+
*
38+
* Generous on purpose. Correlation costs two external calls per app, so an
39+
* instance with 88 enabled apps issues ~176 sequential calls; the whole
40+
* point of doing that here rather than in a request is that there is time
41+
* for it. Ten minutes covers a slow-but-working source while still
42+
* bounding a source that hangs, so one unreachable forge cannot leave a
43+
* cron job running forever.
44+
*
45+
* @var float
46+
*/
47+
private const SWEEP_BUDGET_SECONDS = 600.0;
48+
3449
public function __construct(
3550
ITimeFactory $time,
3651
private AdvisoryService $advisoryService,
3752
private AdvisoryNotifier $advisoryNotifier,
53+
private AdvisoryResultStore $resultStore,
3854
private LoggerInterface $logger,
3955
) {
4056
parent::__construct($time);
@@ -46,7 +62,33 @@ public function __construct(
4662
*/
4763
protected function run($argument): void {
4864
try {
49-
$correlations = $this->advisoryService->correlateAll();
65+
// The sweep passes its OWN budget. AdvisoryService's default is
66+
// sized for a request someone is waiting on (5s); inheriting it
67+
// here would clip this job to the first handful of apps and make
68+
// the stored snapshot — and the notifications derived from it —
69+
// quietly incomplete.
70+
$correlations = $this->advisoryService->correlateAll(self::SWEEP_BUDGET_SECONDS);
71+
72+
// Persist BEFORE notifying. Notification is the more failure-prone
73+
// half (it talks to the notifications app), and a snapshot the
74+
// admin UI can render is worth keeping even if the notification
75+
// half then throws.
76+
$this->resultStore->save($correlations, $this->time->getTime());
77+
78+
$unreached = count(array_filter(
79+
$correlations,
80+
static fn (array $entry): bool => ($entry['error'] ?? null) !== null,
81+
));
82+
if ($unreached > 0) {
83+
// Said out loud, because "correlated" and "correlated except
84+
// for 30 apps whose source did not answer" look identical in
85+
// a UI that only renders badges for what it found.
86+
$this->logger->warning('AdvisoryRefreshJob: some apps could not be correlated', [
87+
'unreached' => $unreached,
88+
'total' => count($correlations),
89+
]);
90+
}
91+
5092
$fired = $this->advisoryNotifier->notifyNewAdvisories($correlations);
5193
if ($fired > 0) {
5294
$this->logger->info('AdvisoryRefreshJob: raised advisory notifications', ['count' => $fired]);

‎lib/Controller/ApiController.php‎

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
use OCA\AppVersions\Db\AuditEntryMapper;
1717
use OCA\AppVersions\Db\Pat;
1818
use OCA\AppVersions\Db\PatMapper;
19-
use OCA\AppVersions\Service\Advisory\AdvisoryService;
19+
use OCA\AppVersions\Service\Advisory\AdvisoryResultStore;
2020
use OCA\AppVersions\Service\AutoUpdate\AutoUpdateSettingsStore;
2121
use OCA\AppVersions\Service\AutoUpdate\AutoUpdateWindow;
2222
use OCA\AppVersions\Service\Cache\ArtifactCache;
@@ -62,7 +62,7 @@ public function __construct(
6262
private PatDeeplinkBuilder $deeplinkBuilder,
6363
private PatExpiryEvaluator $patExpiryEvaluator,
6464
private DiscoveryAggregator $discoveryAggregator,
65-
private AdvisoryService $advisoryService,
65+
private AdvisoryResultStore $advisoryResultStore,
6666
private AuditEntryMapper $auditEntryMapper,
6767
private PinStore $pinStore,
6868
private IAppManager $appManager,
@@ -108,14 +108,28 @@ public function apps(): DataResponse {
108108
}
109109

110110
/**
111-
* Correlates each installed app's version against known security advisories
112-
* (admin-only, read-only). Returns a per-app map of advisory state
111+
* Returns the most recent security-advisory correlation for each installed
112+
* app (admin-only, read-only): a per-app map of advisory state
113113
* (`none` | `advisory-available` | `pinned-to-vulnerable`), the matching
114-
* advisories, and the recommended safe version. Never changes a version
114+
* advisories, and the recommended safe version. Never changes a version.
115115
*
116-
* @return DataResponse<Http::STATUS_OK, array{advisories: array<string, mixed>}, array{}>|DataResponse<Http::STATUS_FORBIDDEN, array{message: string}, array{}>
116+
* READS A SNAPSHOT, DOES NOT COMPUTE ONE. Correlation costs two external
117+
* calls per app — ~176 sequential calls on an 88-app instance — which this
118+
* endpoint used to do inline. Measured on a live instance it then did not
119+
* answer within 120s, twice, and while it held the PHP session lock the
120+
* sibling `/api/pins` request never ran at all, so pin badges silently
121+
* never rendered (issue #160). The sweep now runs in AdvisoryRefreshJob
122+
* every 6 hours and this endpoint serves what it stored.
117123
*
118-
* 200: Advisory correlation returned
124+
* `checkedAt` is part of the contract, not decoration: it is the unix time
125+
* of the last completed sweep, and `null` means no sweep has completed
126+
* yet. Without it the client cannot tell a fresh "no advisories" from a
127+
* six-hour-old one, or from an instance whose cron has never run — three
128+
* states that otherwise render as an identical empty map.
129+
*
130+
* @return DataResponse<Http::STATUS_OK, array{advisories: array<string, mixed>, checkedAt: ?int}, array{}>|DataResponse<Http::STATUS_FORBIDDEN, array{message: string}, array{}>
131+
*
132+
* 200: Stored advisory correlation returned
119133
* 403: Caller is not an administrator
120134
*
121135
* @spec openspec/specs/security-advisory-correlation/spec.md
@@ -126,7 +140,12 @@ public function advisories(): DataResponse {
126140
return new DataResponse(['message' => 'Forbidden'], Http::STATUS_FORBIDDEN);
127141
}
128142

129-
return new DataResponse(['advisories' => $this->advisoryService->correlateAll()]);
143+
$snapshot = $this->advisoryResultStore->read();
144+
145+
return new DataResponse([
146+
'advisories' => $snapshot['advisories'],
147+
'checkedAt' => $snapshot['checkedAt'],
148+
]);
130149
}
131150

132151
/**
Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
/**
5+
* @license EUPL-1.2
6+
* @copyright Copyright (c) 2025, Conduction B.V. <info@conduction.nl>
7+
*
8+
* SPDX-FileCopyrightText: 2025 Conduction B.V. <info@conduction.nl>
9+
* SPDX-License-Identifier: EUPL-1.2
10+
*/
11+
12+
13+
namespace OCA\AppVersions\Service\Advisory;
14+
15+
use OCA\AppVersions\AppInfo\Application;
16+
use OCP\IAppConfig;
17+
use Psr\Log\LoggerInterface;
18+
19+
/**
20+
* Persists the most recent full advisory correlation so the admin UI can read
21+
* a result instead of computing one.
22+
*
23+
* WHY THIS EXISTS. Correlating advisories makes two external calls per app
24+
* (`listAdvisories` + `listVersions`). On an instance with 88 enabled apps
25+
* that is 176 sequential external calls, which is not work a page-load
26+
* endpoint can do: measured on a live instance, `GET /api/advisories` did not
27+
* answer within 120s, twice, and while it held the PHP session lock the
28+
* sibling `/api/pins` request never ran at all (issue #160).
29+
*
30+
* The correlation therefore happens in {@see \OCA\AppVersions\BackgroundJob\AdvisoryRefreshJob},
31+
* which writes here, and the endpoint reads. That keeps the feature's COVERAGE
32+
* — every enabled app is still correlated — and pays for it in staleness
33+
* rather than in a request that cannot return. The stored `checkedAt` is what
34+
* lets the UI say how old the answer is instead of implying it is live.
35+
*
36+
* @psalm-api
37+
*/
38+
class AdvisoryResultStore {
39+
/** App config key holding the JSON-encoded correlation snapshot. */
40+
private const KEY = 'advisory.results';
41+
42+
/** App config key holding the unix time of the last completed sweep. */
43+
private const KEY_CHECKED_AT = 'advisory.results.checkedAt';
44+
45+
public function __construct(
46+
private IAppConfig $config,
47+
private LoggerInterface $logger,
48+
) {
49+
}
50+
51+
/**
52+
* Stores a completed correlation snapshot and the moment it completed.
53+
*
54+
* Only ever called after a sweep finishes, so a half-written snapshot
55+
* never replaces a good one.
56+
*
57+
* @spec openspec/specs/security-advisory-correlation/spec.md
58+
* @param array<string, array{appId: string, installedVersion: ?string, state: string, advisories: list<array{id: string, severity: string, summary: string}>, recommendedVersion: ?string, error: ?string}> $correlations
59+
*/
60+
public function save(array $correlations, int $checkedAt): void {
61+
try {
62+
$encoded = json_encode($correlations, JSON_THROW_ON_ERROR);
63+
} catch (\JsonException $error) {
64+
// A snapshot that cannot be encoded must not clear the previous
65+
// one: a stale answer is worth more than no answer, and the UI
66+
// states its age either way.
67+
$this->logger->error('AdvisoryResultStore: could not encode correlation snapshot; keeping the previous one', [
68+
'message' => $error->getMessage(),
69+
]);
70+
71+
return;
72+
}
73+
74+
$this->config->setValueString(Application::APP_ID, self::KEY, $encoded);
75+
$this->config->setValueInt(Application::APP_ID, self::KEY_CHECKED_AT, $checkedAt);
76+
}
77+
78+
/**
79+
* Reads the stored snapshot. Returns an empty map with a null `checkedAt`
80+
* when no sweep has completed yet — which is a real state on a fresh
81+
* install and must be distinguishable from "swept, found nothing".
82+
*
83+
* @spec openspec/specs/security-advisory-correlation/spec.md
84+
* @return array{advisories: array<string, mixed>, checkedAt: ?int}
85+
*/
86+
public function read(): array {
87+
$raw = $this->config->getValueString(Application::APP_ID, self::KEY, '');
88+
if ($raw === '') {
89+
return ['advisories' => [], 'checkedAt' => null];
90+
}
91+
92+
try {
93+
$decoded = json_decode($raw, true, 512, JSON_THROW_ON_ERROR);
94+
} catch (\JsonException $error) {
95+
$this->logger->warning('AdvisoryResultStore: stored snapshot is not valid JSON; reporting as never checked', [
96+
'message' => $error->getMessage(),
97+
]);
98+
99+
return ['advisories' => [], 'checkedAt' => null];
100+
}
101+
102+
if (!is_array($decoded)) {
103+
return ['advisories' => [], 'checkedAt' => null];
104+
}
105+
106+
$checkedAt = $this->config->getValueInt(Application::APP_ID, self::KEY_CHECKED_AT, 0);
107+
108+
// json_decode yields array-key keys; the keys we wrote are app ids.
109+
// Stated once here rather than re-derived by every caller.
110+
/** @var array<string, mixed> $advisories */
111+
$advisories = $decoded;
112+
113+
return [
114+
'advisories' => $advisories,
115+
// 0 means the key was absent. Reporting it as `null` keeps
116+
// "never checked" one value rather than two.
117+
'checkedAt' => $checkedAt > 0 ? $checkedAt : null,
118+
];
119+
}
120+
}

‎lib/Service/Advisory/AdvisoryService.php‎

Lines changed: 43 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -47,17 +47,24 @@ class AdvisoryService {
4747
public const STATE_NONE = 'none';
4848

4949
/**
50-
* Wall-clock ceiling for a full correlateAll() sweep, in seconds.
50+
* Default wall-clock ceiling for a correlateAll() sweep, in seconds.
5151
*
52-
* Chosen against the frontend that consumes it: App.vue aborts its
53-
* background fetches at 8s, so a server budget above that would be spent
54-
* producing a response nobody is still waiting for. 5s leaves room for the
55-
* response to be serialised and delivered inside that window — a bound must
56-
* fit inside the bound that contains it.
52+
* Sized for a caller someone is waiting on: App.vue aborts its background
53+
* fetches at 8s, so a budget above that would be spent producing a
54+
* response nobody is still waiting for. A bound must fit inside the bound
55+
* that contains it.
56+
*
57+
* NOTE that this default is now a fallback, not the normal path. The
58+
* request path no longer sweeps at all — it reads the snapshot written by
59+
* AdvisoryRefreshJob — and the job passes its own, far larger budget.
60+
* Leaving 5s as the default here would have quietly clipped the JOB to a
61+
* handful of apps, which is the opposite of what a background sweep is
62+
* for: the budget that made the endpoint answerable would have become the
63+
* budget that made the coverage wrong.
5764
*
5865
* @var float
5966
*/
60-
private const CORRELATE_ALL_BUDGET_SECONDS = 5.0;
67+
public const CORRELATE_ALL_BUDGET_SECONDS = 5.0;
6168
public const STATE_AVAILABLE = 'advisory-available';
6269
public const STATE_VULNERABLE = 'pinned-to-vulnerable';
6370

@@ -115,11 +122,16 @@ public function correlate(string $appId): array {
115122
* single unreachable source does not abort the whole sweep.
116123
*
117124
* @spec openspec/specs/security-advisory-correlation/spec.md
125+
* @param ?float $budgetSeconds Wall-clock ceiling for the sweep. Callers a
126+
* user is waiting on should leave this null (see the default). The
127+
* background refresh passes its own, much larger budget so that moving
128+
* the work off the request path does not silently shrink what the
129+
* feature covers.
118130
* @return array<string, array{appId: string, installedVersion: ?string, state: string, advisories: list<array{id: string, severity: string, summary: string}>, recommendedVersion: ?string, error: ?string}>
119131
*/
120-
public function correlateAll(): array {
132+
public function correlateAll(?float $budgetSeconds = null): array {
121133
$results = [];
122-
$deadline = microtime(true) + self::CORRELATE_ALL_BUDGET_SECONDS;
134+
$deadline = microtime(true) + ($budgetSeconds ?? self::CORRELATE_ALL_BUDGET_SECONDS);
123135

124136
foreach ($this->appManager->getEnabledApps() as $appId) {
125137
// BUDGET, BECAUSE THIS ENDPOINT COULD NOT PREVIOUSLY RETURN AT ALL.
@@ -224,14 +236,16 @@ public function evaluate(string $installedVersion, array $advisories, array $ava
224236
* @return list<array{id: string, severity: string, summary: string}>
225237
*/
226238
private function summarise(array $advisories): array {
227-
return array_values(array_map(
239+
// No array_values(): $advisories is already a list, so mapping it
240+
// yields a list.
241+
return array_map(
228242
static fn (array $a): array => [
229243
'id' => $a['id'],
230244
'severity' => $a['severity'],
231245
'summary' => $a['summary'],
232246
],
233247
$advisories,
234-
));
248+
);
235249
}
236250

237251
/**
@@ -270,12 +284,29 @@ private function satisfiesClause(string $version, string $clause): bool {
270284
if (preg_match('/^(<=|>=|<|>|=)?\s*(.+)$/', $clause, $matches) !== 1) {
271285
return false;
272286
}
273-
$operator = $matches[1] !== '' ? $matches[1] : '=';
274287
$bound = trim($matches[2]);
275288
if ($bound === '') {
276289
return false;
277290
}
278291

292+
// version_compare's third argument is a CLOSED SET, and with an
293+
// operator outside it the function returns null rather than a bool.
294+
// The regex above already constrains the input, but naming the set
295+
// here makes that guarantee visible to the type system instead of
296+
// leaving a `bool|null` that only happens to be safe.
297+
$operator = match ($matches[1]) {
298+
'<' => '<',
299+
'<=' => '<=',
300+
'>' => '>',
301+
'>=' => '>=',
302+
// A bare version with no operator means equality.
303+
'=', '' => '=',
304+
default => null,
305+
};
306+
if ($operator === null) {
307+
return false;
308+
}
309+
279310
return version_compare($version, $bound, $operator);
280311
}
281312

‎psalm-baseline.xml‎

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -161,20 +161,6 @@
161161
<code><![CDATA[array_values]]></code>
162162
</RedundantFunctionCall>
163163
</file>
164-
<file src="lib/Service/Advisory/AdvisoryService.php">
165-
<ArgumentTypeCoercion>
166-
<code><![CDATA[$operator]]></code>
167-
</ArgumentTypeCoercion>
168-
<InvalidNullableReturnType>
169-
<code><![CDATA[bool]]></code>
170-
</InvalidNullableReturnType>
171-
<NullableReturnStatement>
172-
<code><![CDATA[version_compare($version, $bound, $operator)]]></code>
173-
</NullableReturnStatement>
174-
<RedundantFunctionCall>
175-
<code><![CDATA[array_values]]></code>
176-
</RedundantFunctionCall>
177-
</file>
178164
<file src="lib/Service/AutoUpdate/AttemptLedger.php">
179165
<MixedAssignment>
180166
<code><![CDATA[$entry]]></code>

0 commit comments

Comments
 (0)