Skip to content

Commit 83ef3d0

Browse files
authored
ui: fix service page not rendering (#28005)
Fixes: #27980
1 parent 3917344 commit 83ef3d0

9 files changed

Lines changed: 291 additions & 43 deletions

File tree

.changelog/28005.txt

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
```release-note:bug
2+
ui: Fix service detail page not rendering
3+
```

ui/app/adapters/watchable.js

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -218,9 +218,36 @@ export default class Watchable extends ApplicationAdapter {
218218
json,
219219
);
220220
if (replace) {
221-
store.unloadAll(relationship.type);
221+
// Capture existing record IDs before pushing new data.
222+
// We must push first so that the relationship's LID references are
223+
// updated before any old records are removed. Calling
224+
// store.unloadAll() first leaves dangling LID references in the
225+
// hasMany relationship state, which causes Ember Data 4.12+ to
226+
// throw "Cannot create a record ... as no resource data exists"
227+
// when the relationship is accessed.
228+
const existingIds = store
229+
.peekAll(relationship.type)
230+
.map((r) => r.id);
231+
const newIds = new Set(
232+
(normalizedData.data || []).map((r) => r.id),
233+
);
234+
235+
store.push(normalizedData);
236+
237+
// Remove records that were not present in the new payload using
238+
// removeRecord, which safely unlinks the record from all of its
239+
// relationships before unloading it from the store.
240+
existingIds.forEach((id) => {
241+
if (!newIds.has(id)) {
242+
const record = store.peekRecord(relationship.type, id);
243+
if (record) {
244+
removeRecord(store, record);
245+
}
246+
}
247+
});
248+
} else {
249+
store.push(normalizedData);
222250
}
223-
store.push(normalizedData);
224251
},
225252
(error) => {
226253
if (error instanceof AbortError || error.name === 'AbortError') {
Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,20 @@
11
/**
2-
* Copyright IBM Corp. 2015, 2025
2+
* Copyright IBM Corp. 2015, 2026
33
* SPDX-License-Identifier: BUSL-1.1
44
*/
55

66
import Helper from '@ember/component/helper';
77

88
export function asyncEscapeHatch([model, relationship]) {
9-
return model[relationship].content;
9+
// Guard against accessing async relationship proxies on records that have
10+
// been unloaded from the store (e.g. after removeRecord). The optional chain
11+
// on the relationship handles the null-belongsTo case set by removeRecord,
12+
// and the try/catch handles the fully-unloaded record case.
13+
try {
14+
return model[relationship]?.content ?? null;
15+
} catch {
16+
return null;
17+
}
1018
}
1119

1220
export default Helper.helper(asyncEscapeHatch);

ui/app/helpers/format-id.js

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,21 @@
11
/**
2-
* Copyright IBM Corp. 2015, 2025
2+
* Copyright IBM Corp. 2015, 2026
33
* SPDX-License-Identifier: BUSL-1.1
44
*/
55

66
import Helper from '@ember/component/helper';
77

88
export function formatID([model, relationship]) {
9-
const id = model.belongsTo(relationship).id();
10-
return { id, shortId: id.split('-')[0] };
9+
// store.unloadRecord() does not set isDestroyed=true on the Ember Object, so
10+
// we cannot use that flag as a guard. Use try/catch instead: if the record
11+
// has been removed from the store its internal identifier is gone and
12+
// belongsTo() will throw an assertion error.
13+
try {
14+
const id = model.belongsTo(relationship).id();
15+
return { id, shortId: id?.split('-')[0] };
16+
} catch {
17+
return { id: null, shortId: null };
18+
}
1119
}
1220

1321
export default Helper.helper(formatID);

ui/app/routes/jobs/job/services/service.js

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
*/
55

66
import Route from '@ember/routing/route';
7+
import { addObserver, removeObserver } from '@ember/object/observers';
78

89
export default class JobsJobServicesServiceRoute extends Route {
910
model({ name = '', level = '' }) {
@@ -14,4 +15,35 @@ export default class JobsJobServicesServiceRoute extends Route {
1415
);
1516
return { name, instances: services || [] };
1617
}
18+
19+
// Watch the parent job's services collection while this detail route is
20+
// active. When the watcher updates job.services (e.g. after
21+
// `nomad service delete`), job.services.length changes and we refresh this
22+
// route so model() reruns with a fresh filtered snapshot. Without this, the
23+
// static `instances` array captured at route entry would keep stale
24+
// references to records that have been unloaded from the store.
25+
//
26+
// The observer is added in activate() and torn down in deactivate(), so its
27+
// lifetime is bounded to the time this route is active. The ember/no-observers
28+
// rule is disabled because we genuinely need cross-route reactivity here.
29+
activate() {
30+
super.activate(...arguments);
31+
const job = this.modelFor('jobs.job');
32+
if (!job) return;
33+
this._servicesObserver = () => {
34+
if (this.isDestroyed || this.isDestroying) return;
35+
this.refresh();
36+
};
37+
// eslint-disable-next-line ember/no-observers
38+
addObserver(job, 'services.length', this._servicesObserver);
39+
}
40+
41+
deactivate() {
42+
const job = this.modelFor('jobs.job');
43+
if (job && this._servicesObserver) {
44+
removeObserver(job, 'services.length', this._servicesObserver);
45+
}
46+
this._servicesObserver = null;
47+
super.deactivate(...arguments);
48+
}
1749
}

ui/app/templates/jobs/job/services/service.hbs

Lines changed: 59 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -16,41 +16,64 @@
1616
{{this.model.name}}
1717
</h1>
1818

19-
<ListTable @source={{this.model.instances}} as |t|>
20-
<t.head>
21-
<th>Allocation</th>
22-
<th>Client</th>
23-
<th>IP Address &amp; Port</th>
24-
</t.head>
25-
<t.body as |row|>
26-
<tr data-test-service-row>
27-
{{#let (format-id row.model "allocation") as |allocation|}}
28-
<td
29-
{{keyboard-shortcut
30-
enumerated=true
31-
action=(fn this.gotoAllocation row.model.allocation)
32-
}}
33-
>
34-
<LinkTo
35-
@route="allocations.allocation"
36-
@model={{allocation.id}}
37-
>{{allocation.shortId}}</LinkTo>
38-
</td>
39-
{{/let}}
40-
{{#let (async-escape-hatch row.model "node") as |node|}}
19+
{{#if this.model.instances.length}}
20+
<ListTable @source={{this.model.instances}} as |t|>
21+
<t.head>
22+
<th>Allocation</th>
23+
<th>Client</th>
24+
<th>IP Address &amp; Port</th>
25+
</t.head>
26+
<t.body as |row|>
27+
<tr data-test-service-row>
28+
{{#let (format-id row.model "allocation") as |allocation|}}
29+
<td
30+
{{keyboard-shortcut
31+
enumerated=true
32+
action=(fn this.gotoAllocation row.model.allocation)
33+
}}
34+
>
35+
{{! Guard: allocation.id can be null if the underlying service
36+
record has been removed from the store (e.g. nomad service
37+
delete). Without this guard LinkTo throws while trying to
38+
generate a URL from a null model. }}
39+
{{#if allocation.id}}
40+
<LinkTo
41+
@route="allocations.allocation"
42+
@model={{allocation.id}}
43+
>{{allocation.shortId}}</LinkTo>
44+
{{/if}}
45+
</td>
46+
{{/let}}
47+
{{#let (async-escape-hatch row.model "node") as |node|}}
48+
<td>
49+
{{! Guard: node can be null when the service's node relationship
50+
has been cleared or unloaded. }}
51+
{{#if node}}
52+
<Tooltip @text={{node.name}}>
53+
<LinkTo
54+
@route="clients.client"
55+
@model={{node.id}}
56+
>{{node.shortId}}</LinkTo>
57+
</Tooltip>
58+
{{/if}}
59+
</td>
60+
{{/let}}
4161
<td>
42-
<Tooltip @text={{node.name}}>
43-
<LinkTo
44-
@route="clients.client"
45-
@model={{node.id}}
46-
>{{node.shortId}}</LinkTo>
47-
</Tooltip>
62+
{{row.model.address}}:{{row.model.port}}
4863
</td>
49-
{{/let}}
50-
<td>
51-
{{row.model.address}}:{{row.model.port}}
52-
</td>
53-
</tr>
54-
</t.body>
55-
</ListTable>
56-
</section>
64+
</tr>
65+
</t.body>
66+
</ListTable>
67+
{{else}}
68+
<div class="boxed-section-body">
69+
<div class="empty-message" data-test-empty-service-instances>
70+
<h3 class="empty-message-headline">No Running Instances</h3>
71+
<p class="empty-message-body">
72+
There are no running registrations for this service. The service is
73+
defined in the job specification but no allocations are currently
74+
registering it.
75+
</p>
76+
</div>
77+
</div>
78+
{{/if}}
79+
</section>

ui/tests/acceptance/job-services-test.js

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { find, findAll, currentURL, settled } from '@ember/test-helpers';
88
import { setupApplicationTest } from 'ember-qunit';
99
import { setupMirage } from 'ember-cli-mirage/test-support';
1010
import { allScenarios } from '../../mirage/scenarios/default';
11+
import removeRecord from 'nomad-ui/utils/remove-record';
1112

1213
import a11yAudit from 'nomad-ui/tests/helpers/a11y-audit';
1314
import Services from 'nomad-ui/tests/pages/jobs/job/services';
@@ -57,4 +58,72 @@ module('Acceptance | job services', function (hooks) {
5758
'Same number of alloc rows as the index shows',
5859
);
5960
});
61+
62+
test('service detail page handles registrations being deleted from under the job', async function (assert) {
63+
const serviceName = find(
64+
'[data-test-service-level="group"][data-test-service-provider="nomad"]',
65+
).getAttribute('data-test-service-name');
66+
const initialCount = Number(
67+
find(
68+
'[data-test-service-level="group"][data-test-service-provider="nomad"]',
69+
).getAttribute('data-test-num-allocs'),
70+
);
71+
72+
await find(
73+
'[data-test-service-level="group"][data-test-service-provider="nomad"] a',
74+
).click();
75+
await settled();
76+
77+
assert.strictEqual(
78+
findAll('tr[data-test-service-row]').length,
79+
initialCount,
80+
'detail page renders the expected number of instances',
81+
);
82+
83+
// Simulate what reloadRelationship + removeRecord do when `nomad service
84+
// delete` runs on the backend: null out relationships (which removes the
85+
// service from job.services via inverse) and unload the record. The route
86+
// observer should pick up the job.services.length change and refresh.
87+
const store = this.owner.lookup('service:store');
88+
const toRemove = store
89+
.peekAll('service')
90+
.find((s) => s.name === serviceName && s.derivedLevel === 'group');
91+
92+
if (toRemove) {
93+
removeRecord(store, toRemove);
94+
}
95+
96+
await settled();
97+
98+
assert.strictEqual(
99+
findAll('tr[data-test-service-row]').length,
100+
initialCount - 1,
101+
'detail page updates after a service registration is removed',
102+
);
103+
assert.dom('.empty-message-headline').doesNotExist();
104+
});
105+
106+
test('service detail page shows empty state when there are no live instances', async function (assert) {
107+
const serviceName = find(
108+
'[data-test-service-level="group"][data-test-service-provider="nomad"]',
109+
).getAttribute('data-test-service-name');
110+
111+
// Remove every live registration for this service before navigating in.
112+
const store = this.owner.lookup('service:store');
113+
store
114+
.peekAll('service')
115+
.filter((s) => s.name === serviceName && s.derivedLevel === 'group')
116+
.forEach((s) => removeRecord(store, s));
117+
await settled();
118+
119+
await find(
120+
'[data-test-service-level="group"][data-test-service-provider="nomad"] a',
121+
).click();
122+
await settled();
123+
124+
assert
125+
.dom('[data-test-empty-service-instances]')
126+
.exists('empty state is shown when no live registrations exist');
127+
assert.dom('tr[data-test-service-row]').doesNotExist();
128+
});
60129
});

ui/tests/unit/adapters/job-test.js

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,73 @@ module('Unit | Adapter | Job', function (hooks) {
278278
);
279279
});
280280

281+
test('reloadRelationship with replace:true pushes new records before removing stale ones, preventing dangling LID references', async function (assert) {
282+
await this.initializeUI();
283+
284+
// Create two services for job-1 in mirage
285+
this.server.create('service', {
286+
id: 'svc-keep',
287+
jobId: 'job-1',
288+
namespace: 'default',
289+
});
290+
this.server.create('service', {
291+
id: 'svc-remove',
292+
jobId: 'job-1',
293+
namespace: 'default',
294+
});
295+
296+
// Load the job and trigger the initial services relationship load
297+
const job = await this.store.findRecord(
298+
'job',
299+
JSON.stringify(['job-1', 'default']),
300+
);
301+
await job.services;
302+
await settled();
303+
304+
assert.strictEqual(
305+
this.store.peekAll('service').length,
306+
2,
307+
'Initially 2 services are in the store',
308+
);
309+
310+
// Simulate a server-side change: svc-remove is gone from the server
311+
this.server.db.services.remove('svc-remove');
312+
313+
// Reload the services relationship with replace: true
314+
await this.store.adapterFor('job').reloadRelationship(job, 'services', {
315+
replace: true,
316+
});
317+
await settled();
318+
319+
assert.notOk(
320+
this.store.peekRecord('service', 'svc-remove'),
321+
'svc-remove is removed from the store after the replace reload',
322+
);
323+
assert.ok(
324+
this.store.peekRecord('service', 'svc-keep'),
325+
'svc-keep is still in the store after the replace reload',
326+
);
327+
328+
// Critically, accessing the job's services relationship must not throw
329+
// the "Cannot create a record ... as no resource data exists" error that
330+
// Ember Data 4.12 raises when a hasMany relationship still holds a dangling
331+
// LID for a record that was removed via store.unloadAll().
332+
let errorThrown = null;
333+
try {
334+
const currentServices = job.hasMany('services').value();
335+
if (currentServices) {
336+
currentServices.forEach((s) => void s.serviceName);
337+
}
338+
} catch (e) {
339+
errorThrown = e;
340+
}
341+
assert.strictEqual(
342+
errorThrown,
343+
null,
344+
'Accessing job.services after a replace reload does not throw',
345+
);
346+
});
347+
281348
test('findAll can be canceled', async function (assert) {
282349
await this.initializeUI();
283350

0 commit comments

Comments
 (0)