Development: Improve Hazelcast cluster stability with build agent client mode
#12051
Conversation
This commit addresses cluster instability issues observed during exams with large numbers of concurrent users. When individual build agents become unresponsive, the entire cluster could experience cascading failures due to default timeout values being too conservative. Changes to CacheConfiguration.java: - Switch from deadline to phi-accrual failure detector (adaptive to network conditions and GC pauses) - Reduce MAX_NO_HEARTBEAT_SECONDS from 60s to 15s for faster detection - Reduce OPERATION_CALL_TIMEOUT_MILLIS from 60s to 15s to prevent thread blocking on unresponsive members - Reduce INVOCATION_MAX_RETRY_COUNT from ~250 to 5 for faster failure - Enable slow operation detection with 5s threshold for diagnostics Changes to HazelcastConnection.java: - Add detection and logging of stale/zombie members (in Hazelcast but not in service registry) - Improve efficiency by using Set instead of List for membership checks Co-Authored-By: Claude Opus 4.5 <[email protected]>
…covery Implements build agents as Hazelcast clients instead of cluster members: - Isolates core cluster from build agent failures (no heartbeat overhead) - Auto-discovers core nodes from service registry (no hardcoded addresses) - Handles reconnection when core nodes are temporarily unavailable - Uses async start with unlimited retry for resilience during startup Co-Authored-By: Claude Opus 4.5 <[email protected]>
Build agents now always connect as Hazelcast clients instead of lite members. This provides better cluster stability by isolating the core cluster from build agent failures and eliminating heartbeat overhead. Removes the build-agent-client-mode configuration flag since client mode is now the default behavior for all build agent deployments. Test profiles (buildagent-test) are excluded from client mode since build agent tests create a local Hazelcast instance. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Upgrades Hazelcast to version 5.6.0 which includes: - Multiple security CVE fixes - Client schema fetching no longer blocks operations - IMap migration state fix for better stability - Enhanced IMap metrics and entry processor performance Excludes HazelcastObjectExtractionConfiguration to work around a known incompatibility with custom Hazelcast configuration beans. See: hazelcast/hazelcast#26553 Also updates RoutingMode import to use the new public package location (com.hazelcast.client.config) instead of the deprecated internal package. Co-Authored-By: Claude Opus 4.5 <[email protected]>
krusche
changed the title
Development: Improve Hazelcast cluster stability with build agent client mode
github-actions
bot
added
server
WalkthroughAdds Hazelcast 5.6.0, client-mode support with Eureka-backed discovery and address normalization, connection lifecycle/listener APIs and retry logic, switches build-agent registry keys to short names, adds build-job DTO/endpoints and WebSocket updates, and introduces many frontend table components, templates, routes, i18n and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Registry as Service Registry (Eureka)
participant Client as Build Agent (Hazelcast Client)
participant Cluster as Hazelcast Cluster
Client->>App: discoverCoreNodeAddresses()
App->>Registry: query core instances / metadata
Registry-->>App: core node addresses
App-->>Client: normalized address list
alt addresses available
Client->>Client: createHazelcastClient(addresses) (asyncStart, backoff)
Client->>App: registerAsClient()
App->>Registry: update metadata (hazelcast.member-type=client)
Registry-->>App: ack
Client->>Cluster: connect as client
Cluster-->>Client: connection established
else no addresses
Client->>Client: start background retry/connect attempts
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/de/tum/cit/aet/artemis/core/config/HazelcastConnection.java (1)
150-189: Use fullhost:portcomparison with IPv6 bracket formatting for member identification.The current code compares cluster members using host-only strings, which fails when multiple service instances run on the same host with different Hazelcast ports. Additionally, IPv6 addresses lack proper bracket formatting in Hazelcast cluster configurations.
Specifically:
- Lines 150-157 extract only host addresses for Hazelcast members, omitting ports
- Lines 162-165 extract only host addresses from the service registry, omitting ports
- This causes comparisons at lines 171-176 and 180-189 to incorrectly treat multiple instances on the same host as duplicates
- Lines 206 and 293-297 build addresses without properly bracketing IPv6 (e.g., stripping brackets then concatenating with port produces invalid formats)
Introduce helper methods to consistently format
host:portwith IPv6 bracket wrapping, and use them across all member comparisons and address building.Proposed refactoring
- var hazelcastMemberAddresses = hazelcastInstance.getCluster().getMembers().stream().map(member -> { - try { - return member.getAddress().getInetAddress().getHostAddress(); - } - catch (UnknownHostException e) { - return "unknown"; - } - }).collect(Collectors.toSet()); + var hazelcastMemberAddresses = hazelcastInstance.getCluster().getMembers().stream().map(member -> { + try { + String host = member.getAddress().getInetAddress().getHostAddress(); + int port = member.getAddress().getPort(); + return formatHostPort(host, port); + } + catch (UnknownHostException e) { + return "unknown"; + } + }).collect(Collectors.toSet()); - Set<String> registryMemberAddresses = new HashSet<>(); - for (ServiceInstance instance : instances) { - registryMemberAddresses.add(instance.getHost().replace("[", "").replace("]", "")); - } + Set<String> registryMemberAddresses = new HashSet<>(); + for (ServiceInstance instance : instances) { + registryMemberAddresses.add(formatInstanceAddress(instance)); + } - var instanceHostClean = instance.getHost().replace("[", "").replace("]", ""); - if (!hazelcastMemberAddresses.contains(instanceHostClean)) { + var instanceAddress = formatInstanceAddress(instance); + if (!hazelcastMemberAddresses.contains(instanceAddress)) { addHazelcastClusterMember(instance, hazelcastInstance.getConfig()); } - var clusterMemberAddress = instance.getHost() + ":" + clusterMemberPort; + var clusterMemberAddress = formatHostPort(instance.getHost().replace("[", "").replace("]", ""), clusterMemberPort); log.info("Adding Hazelcast cluster member {}", clusterMemberAddress); config.getNetworkConfig().getJoin().getTcpIpConfig().addMember(clusterMemberAddress); - String host = instance.getHost().replace("[", "").replace("]", ""); - String port = instance.getMetadata().getOrDefault("hazelcast.port", String.valueOf(hazelcastPort)); - return host + ":" + port; + String host = instance.getHost().replace("[", "").replace("]", ""); + String port = instance.getMetadata().getOrDefault("hazelcast.port", String.valueOf(hazelcastPort)); + return formatHostPort(host, port); + } + + private String formatHostPort(String host, int port) { + return formatHostPort(host, String.valueOf(port)); + } + + private String formatHostPort(String host, String port) { + boolean isIpv6 = host.contains(":"); + return isIpv6 ? "[" + host + "]:" + port : host + ":" + port;
🤖 Fix all issues with AI agents
In `@src/main/java/de/tum/cit/aet/artemis/core/config/CacheConfiguration.java`:
- Around line 385-437: createHazelcastClient currently calls
hazelcastConnection.discoverCoreNodeAddresses() once and starts a client with an
empty address list when none are found, which leaves the client retrying against
nothing; update the implementation to either (A) schedule a periodic task (e.g.,
single-threaded ScheduledExecutorService started from createHazelcastClient or
an init method) that calls hazelcastConnection.discoverCoreNodeAddresses() and,
when addresses are returned, updates the running Hazelcast client via its
ClientNetworkConfig/ClusterService (or gracefully rebuilds the client) and logs
the change, or (B) replace the static discovery with a Hazelcast
DiscoveryStrategy (Hazelcast 5.6.0+) that queries hazelcastConnection
dynamically so the client can obtain addresses at runtime; reference
createHazelcastClient, hazelcastConnection.discoverCoreNodeAddresses,
hazelcastConnection.registerAsClient, and the returned HazelcastInstance when
implementing the periodic refresh or DiscoveryStrategy approach.
src/main/java/de/tum/cit/aet/artemis/core/config/CacheConfiguration.java
Show resolved
Hide resolved
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
github-actions
bot
added
the
programming
Fixes several issues with the Hazelcast client mode implementation: 1. IPv6 address formatting: Core node addresses are now correctly formatted as [ipv6address]:port instead of ipv6address:port 2. Async startup: Build agents now always start asynchronously and connect to the cluster in the background, preventing startup blocking when core nodes are temporarily unavailable 3. Client address resolution: HazelcastDistributedDataProviderService now correctly handles Hazelcast clients by using getLocalEndpoint() instead of getLocalMember() which only works for cluster members Co-Authored-By: Claude Opus 4.5 <[email protected]>
krusche
force-pushed
the
feature/hazelcast-build-agents-as-clients
branch
from
25004ef to
3e316b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/main/java/de/tum/cit/aet/artemis/core/config/CacheConfiguration.java`:
- Around line 329-346: Replace the JVM-wide calls to
ClusterProperty.*.setSystemProperty(...) with instance-scoped calls on your
Hazelcast Config object (e.g., config.setProperty(...)); specifically, change
each ClusterProperty.HEARTBEAT_INTERVAL_SECONDS.setSystemProperty("5"),
ClusterProperty.MAX_NO_HEARTBEAT_SECONDS.setSystemProperty("15"),
ClusterProperty.OPERATION_CALL_TIMEOUT_MILLIS.setSystemProperty("15000"),
ClusterProperty.OPERATION_BACKUP_TIMEOUT_MILLIS.setSystemProperty("5000"),
ClusterProperty.INVOCATION_MAX_RETRY_COUNT.setSystemProperty("5"), and
ClusterProperty.INVOCATION_RETRY_PAUSE.setSystemProperty("1000") to use
config.setProperty(ClusterProperty.XYZ.getName(), "<value>") so the settings are
scoped to the Config instance rather than JVM-wide system properties.
In `@src/main/java/de/tum/cit/aet/artemis/core/config/HazelcastConnection.java`:
- Around line 310-315: The current registerAsClient() uses
registration.get().getMetadata().put(...) which only mutates the local map and
doesn't propagate to Eureka immediately; replace that local map mutation with a
call to Eureka's API (e.g.
ApplicationInfoManager.registerAppMetadata(Map<String,String>) or equivalent) to
push the HAZELCAST_MEMBER_TYPE_KEY -> HAZELCAST_MEMBER_TYPE_CLIENT update so
other nodes see the change immediately; apply the same fix in CacheConfiguration
where HAZELCAST_MEMBER_TYPE_MEMBER is set, ensuring you locate and update uses
of registration, HAZELCAST_MEMBER_TYPE_KEY, HAZELCAST_MEMBER_TYPE_CLIENT and the
analogous member constant to call ApplicationInfoManager.registerAppMetadata()
(or the Eureka REST endpoint) instead of directly mutating the metadata map.
In
`@src/main/java/de/tum/cit/aet/artemis/programming/service/localci/distributed/hazelcast/HazelcastDistributedDataProviderService.java`:
- Around line 72-95: getLocalMemberAddress returns inconsistent formats: the
Hazelcast client path (HazelcastClientProxy + getLocalEndpoint()) returns
"[host]:port" but the cluster member path uses
hazelcastInstance.getCluster().getLocalMember().getAddress().toString() which
yields "host:port"; update the cluster-member branch (use the local member
Address API like getHost()/getPort() or getInetAddress()/getPort()) to format as
"[host]:port" to match the client path, and adjust the Javadoc on
getLocalMemberAddress to explicitly state the normalized "[host]:port" format.
- Around line 86-91: The code in HazelcastDistributedDataProviderService assumes
hazelcastInstance.getLocalEndpoint() is non-null for HazelcastClientProxy, which
can NPE when asyncStart=true; update the branch that checks (hazelcastInstance
instanceof HazelcastClientProxy) to first fetch and null-check
getLocalEndpoint(), and if it's null return a safe fallback (e.g., "unavailable"
or empty string) instead of dereferencing; if non-null continue to obtain the
socket address via getSocketAddress() and format as "[host]:port".
src/main/java/de/tum/cit/aet/artemis/core/config/CacheConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/config/HazelcastConnection.java
Show resolved
Hide resolved
...ogramming/service/localci/distributed/hazelcast/HazelcastDistributedDataProviderService.java
Show resolved
Hide resolved
...ogramming/service/localci/distributed/hazelcast/HazelcastDistributedDataProviderService.java
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/main/java/de/tum/cit/aet/artemis/programming/service/localci/distributed/hazelcast/HazelcastDistributedDataProviderService.java`:
- Around line 74-101: Update the Javadoc on
HazelcastDistributedDataProviderService's method that returns the unique
instance identifier to match actual behavior: state that memberAddress.getHost()
may be a hostname (not strictly an IP) and clarify the fallback format is
"hostname/instanceName" (not just the instance name) when the client endpoint
isn't connected (asyncStart=true); update the examples to show this fallback and
adjust the address description to say host (IP or hostname) is wrapped in
brackets per RFC 3986, e.g. "[192.168.1.1]:5701", "[2001:db8::1]:5701", and
"my-host.example.com/Artemis-client" for the not-yet-connected client.
- Around line 113-122: The code in HazelcastDistributedDataProviderService
performs an unchecked cast of localEndpoint.getSocketAddress() to
InetSocketAddress; add an instanceof guard to verify the SocketAddress is an
InetSocketAddress before casting (check localEndpoint.getSocketAddress()
instanceof InetSocketAddress), and if it is not, fall back to calling
getUniqueInstanceIdentifier() (or otherwise handle the unexpected SocketAddress
safely) so the method never throws ClassCastException when building the
host:port string.
🧹 Nitpick comments (3)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/distributed/hazelcast/HazelcastDistributedDataProviderService.java (2)
136-146: Consider extra entropy in the fallback identifier to prevent collisions.
If multiple build agents run on the same host with the same instance name,hostname/instanceNamewill collide until the client connects. Consider appending a per‑process suffix (PID/UUID) or another stable unique token.
15-15: Replace the internalHazelcastClientProxycheck with public API endpoint type checking.Line 15 imports an internal class from
com.hazelcast.client.impl. In Hazelcast 5.6, detect client vs. member using the public API: callgetLocalEndpoint()and check whether it is aMemberorClientinstance. This approach avoids coupling to internal impl classes and is stable across upgrades.// Instead of: if (hazelcastInstance instanceof HazelcastClientProxy) { ... } // Use: var localEndpoint = hazelcastInstance.getLocalEndpoint(); if (!(localEndpoint instanceof Member)) { // Handle client } else { // Handle member }src/main/java/de/tum/cit/aet/artemis/core/config/HazelcastConnection.java (1)
150-189: Compare members usinghost:portto avoid collisions on shared hosts.Both
hazelcastMemberAddressesandregistryMemberAddressescurrently track only hosts. InlocalInstancesor multi-node same-host setups, distinct members can collapse into one entry, so late-joining members may be skipped and stale-member warnings can be noisy. Consider storing formattedhost:portaddresses for comparisons.♻️ Proposed refactor
- var hazelcastMemberAddresses = hazelcastInstance.getCluster().getMembers().stream().map(member -> { - try { - return member.getAddress().getInetAddress().getHostAddress(); - } - catch (UnknownHostException e) { - return "unknown"; - } - }).collect(Collectors.toSet()); + var hazelcastMemberAddresses = hazelcastInstance.getCluster().getMembers().stream().map(member -> { + try { + String host = normalizeHost(member.getAddress().getInetAddress().getHostAddress()); + String port = String.valueOf(member.getAddress().getPort()); + return formatAddressForHazelcast(host, port); + } + catch (UnknownHostException e) { + return "unknown"; + } + }).collect(Collectors.toSet()); @@ - Set<String> registryMemberAddresses = new HashSet<>(); - for (ServiceInstance instance : instances) { - registryMemberAddresses.add(normalizeHost(instance.getHost())); - } + Set<String> registryMemberAddresses = new HashSet<>(); + for (ServiceInstance instance : instances) { + registryMemberAddresses.add(formatInstanceAddress(instance)); + } @@ - var instanceHostClean = normalizeHost(instance.getHost()); - if (!hazelcastMemberAddresses.contains(instanceHostClean)) { + var instanceAddress = formatInstanceAddress(instance); + if (!hazelcastMemberAddresses.contains(instanceAddress)) { addHazelcastClusterMember(instance, hazelcastInstance.getConfig()); }
| /** | ||
| * @return the address of the local Hazelcast member | ||
| * Returns a unique identifier for the local Hazelcast instance. | ||
| * For cluster members, returns the member's cluster address in normalized format. | ||
| * For clients (e.g., build agents in client mode), returns the client's local endpoint address, | ||
| * or the instance name as a fallback if not yet connected. | ||
| * | ||
| * <p> | ||
| * The address format is always {@code [host]:port} where the host is the IP address. | ||
| * IPv6 addresses are wrapped in brackets per RFC 3986, and IPv4 addresses are also | ||
| * wrapped in brackets for consistency across the codebase. | ||
| * | ||
| * <p> | ||
| * <strong>Examples:</strong> | ||
| * <ul> | ||
| * <li>IPv4: {@code [192.168.1.1]:5701}</li> | ||
| * <li>IPv6: {@code [2001:db8::1]:5701}</li> | ||
| * <li>Client not connected: {@code Artemis-client} (uses instance name as fallback)</li> | ||
| * </ul> | ||
| * | ||
| * <p> | ||
| * <strong>Important:</strong> When using asyncStart=true for Hazelcast clients, the endpoint | ||
| * address may not be available immediately. In this case, the instance name is returned as a | ||
| * unique identifier. This ensures that build agents can register themselves in the distributed | ||
| * map even before connecting to the cluster. | ||
| * | ||
| * @return a unique identifier for this instance: the address in format {@code [host]:port}, | ||
| * or the instance name if the client endpoint is not yet connected | ||
| * @throws HazelcastInstanceNotActiveException if the Hazelcast instance is not running |
There was a problem hiding this comment.
Align the Javadoc with the actual fallback identifier format.
Line 90 shows only the instance name, but the implementation returns hostname/instanceName, and Line 81 states “IP address” while memberAddress.getHost() may be a hostname. Please update the text/examples to match behavior.
✏️ Suggested doc fix
- * For clients (e.g., build agents in client mode), returns the client's local endpoint address,
- * or the instance name as a fallback if not yet connected.
+ * For clients (e.g., build agents in client mode), returns the client's local endpoint address,
+ * or a hostname/instance-name fallback if not yet connected.
@@
- * The address format is always {`@code` [host]:port} where the host is the IP address.
+ * The address format is always {`@code` [host]:port} where the host is the resolved host (IP or hostname).
@@
- * <li>Client not connected: {`@code` Artemis-client} (uses instance name as fallback)</li>
+ * <li>Client not connected: {`@code` build-agent-01/Artemis-client} (uses hostname + instance name)</li>
@@
- * `@return` a unique identifier for this instance: the address in format {`@code` [host]:port},
- * or the instance name if the client endpoint is not yet connected
+ * `@return` a unique identifier for this instance: the address in format {`@code` [host]:port},
+ * or hostname/instance-name if the client endpoint is not yet connected🤖 Prompt for AI Agents
In
`@src/main/java/de/tum/cit/aet/artemis/programming/service/localci/distributed/hazelcast/HazelcastDistributedDataProviderService.java`
around lines 74 - 101, Update the Javadoc on
HazelcastDistributedDataProviderService's method that returns the unique
instance identifier to match actual behavior: state that memberAddress.getHost()
may be a hostname (not strictly an IP) and clarify the fallback format is
"hostname/instanceName" (not just the instance name) when the client endpoint
isn't connected (asyncStart=true); update the examples to show this fallback and
adjust the address description to say host (IP or hostname) is wrapped in
brackets per RFC 3986, e.g. "[192.168.1.1]:5701", "[2001:db8::1]:5701", and
"my-host.example.com/Artemis-client" for the not-yet-connected client.
| var localEndpoint = hazelcastInstance.getLocalEndpoint(); | ||
| if (localEndpoint == null || localEndpoint.getSocketAddress() == null) { | ||
| // Use hostname + instance name as a unique fallback identifier. | ||
| // This ensures each build agent has a unique key in the distributed map | ||
| // even before connecting to the cluster. | ||
| return getUniqueInstanceIdentifier(); | ||
| } | ||
| InetSocketAddress socketAddress = (InetSocketAddress) localEndpoint.getSocketAddress(); | ||
| // Format as [host]:port for consistency | ||
| return "[" + socketAddress.getAddress().getHostAddress() + "]:" + socketAddress.getPort(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /root && find . -name "pom.xml" -o -name "build.gradle" | head -5Repository: ls1intum/Artemis
Length of output: 115
🏁 Script executed:
cat /root/pom.xml | grep -A 5 "hazelcast"Repository: ls1intum/Artemis
Length of output: 105
🏁 Script executed:
cat -n /root/src/main/java/de/tum/cit/aet/artemis/programming/service/localci/distributed/hazelcast/HazelcastDistributedDataProviderService.java | sed -n '100,130p'Repository: ls1intum/Artemis
Length of output: 229
🌐 Web query:
Hazelcast 5 ClientEndpoint getSocketAddress return type
💡 Result:
In Hazelcast 5.x:
com.hazelcast.client.Client#getSocketAddress()returnsjava.net.InetSocketAddress. [1]- The superinterface
com.hazelcast.cluster.Endpoint#getSocketAddress()is declared asjava.net.SocketAddress, andClientnarrows it covariantly toInetSocketAddress. [2]
Sources: [1] [2]
🏁 Script executed:
git ls-files | grep -E "(pom\.xml|HazelcastDistributedDataProviderService\.java)" | head -10Repository: ls1intum/Artemis
Length of output: 791
🏁 Script executed:
git ls-files | grep pom.xml | head -5Repository: ls1intum/Artemis
Length of output: 392
🏁 Script executed:
git ls-files | grep -E "^pom\.xml$"Repository: ls1intum/Artemis
Length of output: 42
🏁 Script executed:
cat -n src/main/java/de/tum/cit/aet/artemis/programming/service/localci/distributed/hazelcast/HazelcastDistributedDataProviderService.java | sed -n '100,130p'Repository: ls1intum/Artemis
Length of output: 1889
🏁 Script executed:
cat pom.xml | grep -A 2 -B 2 "hazelcast" | head -30Repository: ls1intum/Artemis
Length of output: 99
🏁 Script executed:
find . -maxdepth 2 -name "pom.xml" -type f | grep -v templates | head -5Repository: ls1intum/Artemis
Length of output: 42
🏁 Script executed:
cat -n src/main/java/de/tum/cit/aet/artemis/programming/service/localci/distributed/hazelcast/HazelcastDistributedDataProviderService.java | sed -n '1,40p'Repository: ls1intum/Artemis
Length of output: 2082
🏁 Script executed:
find . -type f \( -name "pom.xml" -o -name "build.gradle" \) ! -path "*/templates/*" ! -path "*/target/*" | head -10Repository: ls1intum/Artemis
Length of output: 131
🏁 Script executed:
rg "hazelcast" --type xml -A 2 -B 2 | head -50Repository: ls1intum/Artemis
Length of output: 820
🏁 Script executed:
cat build.gradle | grep -A 5 -B 5 "hazelcast"Repository: ls1intum/Artemis
Length of output: 1306
🏁 Script executed:
grep "hazelcast_version\s*=" build.gradleRepository: ls1intum/Artemis
Length of output: 42
🏁 Script executed:
rg "getLocalEndpoint|getSocketAddress" --type java -B 3 -A 3 | head -100Repository: ls1intum/Artemis
Length of output: 4353
🏁 Script executed:
grep -E "hazelcast_version\s*=" build.gradle | head -5Repository: ls1intum/Artemis
Length of output: 42
🏁 Script executed:
rg "hazelcast_version" --type gradle build.gradleRepository: ls1intum/Artemis
Length of output: 198
🏁 Script executed:
cat build.gradle | head -100 | grep -E "hazelcast|version"Repository: ls1intum/Artemis
Length of output: 1327
🏁 Script executed:
cat build.gradle | grep -E "^(ext|def).*hazelcast" -A 2Repository: ls1intum/Artemis
Length of output: 42
🏁 Script executed:
rg "hazelcast.*=" build.gradle | head -20Repository: ls1intum/Artemis
Length of output: 42
🏁 Script executed:
find . -name "gradle.properties" -o -name "*.gradle" | grep -v templates | xargs grep -l "hazelcast_version" 2>/dev/nullRepository: ls1intum/Artemis
Length of output: 94
🏁 Script executed:
cat build.gradle | grep -B 5 -A 5 'hazelcast.*version'Repository: ls1intum/Artemis
Length of output: 1304
🏁 Script executed:
cat gradle.properties | grep hazelcast_versionRepository: ls1intum/Artemis
Length of output: 83
Guard the SocketAddress cast.
Endpoint#getSocketAddress() declares return type as SocketAddress, though Client implementations return InetSocketAddress. The unchecked cast could throw if the actual implementation differs. Add an instanceof guard to safely handle unexpected types.
🛡️ Safer cast
- InetSocketAddress socketAddress = (InetSocketAddress) localEndpoint.getSocketAddress();
- // Format as [host]:port for consistency
- return "[" + socketAddress.getAddress().getHostAddress() + "]:" + socketAddress.getPort();
+ var socketAddress = localEndpoint.getSocketAddress();
+ if (!(socketAddress instanceof InetSocketAddress inetSocketAddress)) {
+ return getUniqueInstanceIdentifier();
+ }
+ // Format as [host]:port for consistency
+ return "[" + inetSocketAddress.getAddress().getHostAddress() + "]:" + inetSocketAddress.getPort();🤖 Prompt for AI Agents
In
`@src/main/java/de/tum/cit/aet/artemis/programming/service/localci/distributed/hazelcast/HazelcastDistributedDataProviderService.java`
around lines 113 - 122, The code in HazelcastDistributedDataProviderService
performs an unchecked cast of localEndpoint.getSocketAddress() to
InetSocketAddress; add an instanceof guard to verify the SocketAddress is an
InetSocketAddress before casting (check localEndpoint.getSocketAddress()
instanceof InetSocketAddress), and if it is not, fall back to calling
getUniqueInstanceIdentifier() (or otherwise handle the unexpected SocketAddress
safely) so the method never throws ClassCastException when building the
host:port string.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
- Add REST endpoints to retrieve single build job by ID (admin and course-scoped) - Add WebSocket topics for individual build job updates with security - Create BuildJobDetailComponent with live status updates via WebSocket - Simplify finished builds table by removing unnecessary columns - Make table rows clickable to navigate to job detail page - Auto-load build logs when job finishes - Add server integration tests and client unit tests (Vitest) Co-Authored-By: Claude Opus 4.5 <[email protected]>
github-actions
bot
added
client
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In
`@src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java`:
- Around line 70-88: In getBuildJobById, trim and validate the incoming
buildJobId up front (e.g., String normalizedId = buildJobId == null ? "" :
buildJobId.trim()), and if normalizedId.isBlank() return
ResponseEntity.badRequest().build(); then use normalizedId (not the raw
parameter) in the log statement and all lookups
(distributedDataAccessService.getDistributedProcessingJobs().get(normalizedId),
queuedJob.id().equals(normalizedId), and
buildJobRepository.findByBuildJobId(normalizedId)) and when creating the
FinishedBuildJobDTO so you never perform scans or log noise on blank/whitespace
IDs.
In
`@src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildJobQueueResource.java`:
- Around line 78-99: In getBuildJobById (BuildJobQueueResource) validate the
inputs up front: ensure courseId is positive and buildJobId is not null/blank;
normalize buildJobId (e.g., trim and apply any canonical casing used elsewhere)
before any lookups; if validation fails return
ResponseEntity.badRequest().build() to avoid scanning
distributedDataAccessService.getDistributedProcessingJobs(), getQueuedJobs(), or
buildJobRepository with invalid values; then use the normalized buildJobId for
comparisons against BuildJobQueueItem.id()/courseId() and for the repository
query and mapping to FinishedBuildJobDTO.
In
`@src/main/webapp/app/buildagent/build-agent-details/build-agent-details.component.ts`:
- Around line 82-84: The agentNotFound signal is currently set for all errors
and never cleared; update the fetch/load logic that reads the agent (the method
that sets agentNotFound, referenced by agentNotFound signal) so that on a
successful response you explicitly reset agentNotFound to false, and on error
only set agentNotFound to true when the HTTP status is 404 while handling
non-404 errors separately (do not set agentNotFound for other failures);
additionally, when a 404 occurs clear any stale agent data/state so the UI does
not show old agent info; apply the same changes to the other fetch path noted
around the block at lines 260-272 which uses the same agentNotFound signal.
- Around line 268-271: The error path is passing this.agentName to
FinishedBuildJobFilter which the backend filters by buildAgentAddress; update
the error path so FinishedBuildJobFilter is constructed with the agent address
instead of the name — use this.buildAgent()?.buildAgent?.memberAddress (the same
value used in the success path) when available, then call
loadFinishedBuildJobs(); keep agentNotFound.set(true) as is and only fall back
to this.agentName if no address is present.
In
`@src/main/webapp/app/buildagent/build-job-statistics/build-job-statistics.component.html`:
- Around line 8-17: The radio inputs in the build job statistics template use
static ids ("option1"/"option2"/"option3") which can duplicate if the component
is rendered multiple times; update the template around the inputs that call
onTabChange(SpanType.DAY/WEEK/MONTH) and reference currentSpan to either remove
the id attributes entirely (since labels are not using for=) or generate unique
ids per component instance (e.g., append a component-provided unique prefix),
ensuring no duplicate HTML ids remain.
- Around line 23-55: The pie chart still shows the "Missing" slice because
pieChartData() always includes it; update the logic that builds chart data
(e.g., pieChartData() or updateDisplayedBuildJobStatistics()) to filter out or
omit the missing-builds entry when displayMissingBuilds is false so the chart
and the stats list stay consistent; locate where pieChartData() assembles the
array (and where buildJobStatistics()/displayMissingBuilds are referenced) and
conditionally exclude the object for missingBuilds or rebuild the chart data
from the displayedBuildJobStatistics only when displayMissingBuilds is false.
In
`@src/main/webapp/app/buildagent/build-queue/build-job-detail/build-job-detail.component.html`:
- Around line 214-219: The template uses the JS typeof operator (typeof
buildDuration() === 'number') which is not supported in Angular templates;
update the component to expose a helper boolean method (e.g.
isBuildDurationNumeric()) that returns typeof this.buildDuration() === 'number'
or refactor buildDuration() to always return a consistent type, then replace the
typeof expression in build-job-detail.component.html with a call to the helper
(isBuildDurationNumeric()) so the template condition works without using typeof.
In `@src/main/webapp/app/buildagent/build-queue/build-overview.component.html`:
- Around line 95-106: The agent link to admin details is rendered
unconditionally; change the cell template in build-overview.component.html to
conditionally render the <a> only when isAdministrationView() returns true and
render plain text otherwise (use Angular's *ngIf with an else template or the
short-circuit pattern). Locate the ngx-datatable-cell-template (where value and
row are available) and wrap the current routerLink anchor with
*ngIf="isAdministrationView()", moving the current display into that branch, and
add an else template (e.g., plainAgent) that outputs {{ value }} as plain text;
keep the existing bindings (row.buildAgent!.name and value) and the header sort
handlers (controls.onSort, controls.iconForSortPropField) untouched.
- Around line 271-285: The icon-only cancel buttons inside the
ngx-datatable-cell-template need accessible names: add aria-label="Cancel build
job" to the button that calls cancelBuildJob(row.id) (the element showing
<fa-icon [icon]="faTimes" />) while leaving the header cancel button (which
calls cancelAllRunningBuildJobs() and already contains visible text) unchanged;
additionally, replace the <a> element used as a click-only control with a proper
<button type="button"> to ensure keyboard focus and correct semantics.
In `@src/main/webapp/app/buildagent/build-queue/build-overview.component.ts`:
- Around line 183-184: The component calls loadBuildAgents() and subscribes to
the /topic/admin/build-agents topic unconditionally, which causes 403s for
non-admin course views; guard these admin-only operations by checking the
administration mode (use this.isAdministrationView() or
this.isAdministrationView.get() as appropriate) before calling loadBuildAgents()
and before creating the WebSocket subscription to "/topic/admin/build-agents".
Also add error handling to loadBuildAgents() similar to other REST calls in this
component: catch HTTP errors and ignore or silently handle 403 responses (log at
debug level or return an empty result) so non-admin users don’t surface errors.
Locate references to loadBuildAgents(), the WebSocket subscription to
"/topic/admin/build-agents", and any initialization code (where
this.isAdministrationView is set) to apply these guards and the error handling.
🧹 Nitpick comments (13)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java (1)
64-72: Consider applyingremoveUnnecessaryInformationfor consistency.Other websocket methods in this class (e.g.,
sendQueuedJobsOverWebsocket,sendProcessingJobsOverWebsocket) strip sensitive/unnecessary data before sending. This method sends the rawBuildJobQueueItem. If the build job detail page needs the full information, this is fine, but verify that no sensitive data (like repository URIs or build scripts) is inadvertently exposed.src/main/java/de/tum/cit/aet/artemis/core/service/connectors/HazelcastHealthIndicator.java (2)
46-68: Consider capping per-client detail size in health output.On large clusters, iterating and emitting per-client details on every health check can create big payloads and extra CPU. Consider a configurable cap (or only include the count by default, with an optional flag for full details).
🔧 Example cap to limit payload size
- int clientIndex = 1; + final int maxClientsInHealthDetails = 50; // consider externalizing to config + int clientIndex = 1; for (var client : connectedClients) { + if (clientIndex > maxClientsInHealthDetails) { + details.put("Connected clients (truncated)", "Showing first " + maxClientsInHealthDetails); + break; + } String clientInfo = "Client " + clientIndex + " (uuid " + client.getUuid() + ")"; String clientDetails = "Type: " + client.getClientType() + ", Address: " + client.getSocketAddress() + ", Name: " + client.getName(); details.put(clientInfo, clientDetails); clientIndex++; }
13-13: Remove dependency on internal HazelcastClientProxy API.
HazelcastClientProxyis incom.hazelcast.*.impl.*and marked@PrivateApi—it may change or be removed in future releases. The publicHazelcastInstance.getClientService()API provides the same functionality with graceful fallback viaUnsupportedOperationExceptionfor client-mode instances. Replace theinstanceofcheck with exception handling:-import com.hazelcast.client.impl.clientside.HazelcastClientProxy; ... - if (!(hazelcastInstance instanceof HazelcastClientProxy)) { - try { - var clientService = hazelcastInstance.getClientService(); - var connectedClients = clientService.getConnectedClients(); - details.put("Connected clients", String.valueOf(connectedClients.size())); - - int clientIndex = 1; - for (var client : connectedClients) { - String clientInfo = "Client " + clientIndex + " (uuid " + client.getUuid() + ")"; - String clientDetails = "Type: " + client.getClientType() + ", Address: " + client.getSocketAddress() + ", Name: " + client.getName(); - details.put(clientInfo, clientDetails); - clientIndex++; - } - } - catch (UnsupportedOperationException e) { - // Client service not available (e.g., on Hazelcast clients) - details.put("Connected clients", "N/A (running as client)"); - } - } - else { - details.put("Instance type", "Hazelcast Client"); - } + try { + var clientService = hazelcastInstance.getClientService(); + var connectedClients = clientService.getConnectedClients(); + details.put("Connected clients", String.valueOf(connectedClients.size())); + + int clientIndex = 1; + for (var client : connectedClients) { + String clientInfo = "Client " + clientIndex + " (uuid " + client.getUuid() + ")"; + String clientDetails = "Type: " + client.getClientType() + ", Address: " + client.getSocketAddress() + ", Name: " + client.getName(); + details.put(clientInfo, clientDetails); + clientIndex++; + } + } + catch (UnsupportedOperationException e) { + // Client service not available (e.g., on Hazelcast clients) + details.put("Connected clients", "N/A (running as client)"); + details.put("Instance type", "Hazelcast Client"); + }src/main/webapp/app/buildagent/build-queue/build-overview.service.ts (1)
195-210: Use a concrete return type instead ofany.
These endpoints return a known shape (queued/running vs finished). A union type keeps 100% type safety and aligns with the rest of the service.♻️ Suggested refactor
-import { BuildJob, BuildJobStatistics, FinishedBuildJob, SpanType } from 'app/buildagent/shared/entities/build-job.model'; +import { BuildJob, BuildJobStatistics, FinishedBuildJob, SpanType } from 'app/buildagent/shared/entities/build-job.model'; + +type BuildJobDetails = BuildJob | FinishedBuildJob; @@ - getBuildJobById(buildJobId: string): Observable<any> { - return this.http.get<any>(`${this.adminResourceUrl}/build-job/${buildJobId}`); + getBuildJobById(buildJobId: string): Observable<BuildJobDetails> { + return this.http.get<BuildJobDetails>(`${this.adminResourceUrl}/build-job/${buildJobId}`); } @@ - getBuildJobByIdForCourse(courseId: number, buildJobId: string): Observable<any> { - return this.http.get<any>(`${this.resourceUrl}/courses/${courseId}/build-job/${buildJobId}`); + getBuildJobByIdForCourse(courseId: number, buildJobId: string): Observable<BuildJobDetails> { + return this.http.get<BuildJobDetails>(`${this.resourceUrl}/courses/${courseId}/build-job/${buildJobId}`); }As per coding guidelines "prefer 100% type safety".
src/test/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentIntegrationTest.java (1)
114-118: Consider null-guarding build-agent lookups in the polling lambdas.
If the map entry hasn’t been populated yet, dereferencingbuildAgentcan throw and make the poll fail prematurely.♻️ Suggested null-guard pattern (apply similarly in the other blocks)
- var buildAgent = buildAgentInformation.get(buildAgentShortName); - return buildAgent.numberOfCurrentBuildJobs() == 1 && buildAgent.maxNumberOfConcurrentBuildJobs() == 2 && buildAgent.runningBuildJobs().size() == 1 + var buildAgent = buildAgentInformation.get(buildAgentShortName); + return buildAgent != null && buildAgent.numberOfCurrentBuildJobs() == 1 && buildAgent.maxNumberOfConcurrentBuildJobs() == 2 + && buildAgent.runningBuildJobs().size() == 1 && buildAgent.runningBuildJobs().getFirst().id().equals(queueItem.id());Also applies to: 163-168, 291-294, 307-310, 332-335, 339-342, 411-414, 418-421
src/main/webapp/app/buildagent/build-queue/build-job-detail/build-job-detail.component.spec.ts (4)
126-134: Test does not verify course view behavior.The test description says "should handle course view with route params" but the assertion verifies the admin topic is used (
/topic/admin/build-job/...). Since the mock route hascourseId: null, it's actually testing admin view behavior. Consider either renaming the test or creating a separate test that properly mocks a courseId to verify course-specific topic subscription.
136-142: Test does not actually test missing job ID scenario.The mock route provides
jobId: 'test-job-1', so this test verifies behavior with a valid job ID. To properly test the "missing job ID" case, the route mock should returnnullforjobId. Consider creating a new TestBed configuration or parameterizing the mock.
232-244: Download test should verify URL operations were invoked.The test mocks
URL.createObjectURLandURL.revokeObjectURLbut only comments "No assertion needed - just verify it doesn't throw". Adding assertions would improve test quality.Suggested improvement
component.downloadBuildLogs(); - // No assertion needed - just verify it doesn't throw + expect(createObjectURLSpy).toHaveBeenCalled(); });
246-250: Destroy test could verify cleanup more explicitly.Consider verifying that the WebSocket subscription is unsubscribed and the interval is cleared. You could spy on
clearIntervalor check thatwsSubscription?.unsubscribe()was called.src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (1)
432-444: Port-matching heuristic has edge case risk.The logic identifies cluster members by checking if the address port matches any known cluster member port. While this works in typical setups (cluster members use configured ports like 5701, clients use ephemeral ports), there's a theoretical edge case: a client could be assigned an ephemeral port that coincidentally matches a cluster member's port (e.g., if a cluster member uses port 54321 and a client happens to get the same ephemeral port from the OS).
This is unlikely in practice since Hazelcast defaults to well-known ports (5701-5703), but consider documenting this assumption explicitly or adding a comment about the expected port ranges.
src/main/webapp/app/buildagent/build-queue/build-job-detail/build-job-detail.component.ts (3)
45-46: Avoidanytype for better type safety.The
buildJobsignal is typed asany | undefined, which loses type safety. Per coding guidelines, prefer 100% type safety. Consider creating a union type or using a more specific type.Suggested improvement
- /** The build job data - either a BuildJob (queued/running) or FinishedBuildJob */ - buildJob = signal<any | undefined>(undefined); + /** The build job data - either a BuildJob (queued/running) or FinishedBuildJob */ + buildJob = signal<BuildJob | FinishedBuildJob | undefined>(undefined);You may need to import
FinishedBuildJoband potentially create a type guard or discriminated union to handle the differences between the two types in your helper methods.
286-304: Return types should be explicit instead ofany.The methods
getSubmissionDate(),getBuildStartDate(), andgetBuildCompletionDate()returnany. These should return a more specific type likedayjs.Dayjs | string | undefinedor the actual date type used in the models.
97-113: Inconsistent return types inbuildDurationcomputed.The computed signal returns either a formatted string (e.g.,
"1.234s") or a number fromjobTimingInfo.buildDuration, creating type inconsistency. This also relates to the template issue wheretypeofis used to distinguish between them. Consider normalizing the return type.Suggested approach
Either always return a number (seconds) and let the template pipe handle formatting:
buildDuration = computed((): number | undefined => { const job = this.buildJob(); if (!job) return undefined; if (job.buildStartDate && job.buildCompletionDate) { return dayjs(job.buildCompletionDate).diff(dayjs(job.buildStartDate), 'seconds'); } if (job.jobTimingInfo?.buildDuration) { return typeof job.jobTimingInfo.buildDuration === 'number' ? job.jobTimingInfo.buildDuration : undefined; } // ... handle other cases return undefined; });Or always return a string and remove the template type check.
| public ResponseEntity<?> getBuildJobById(@PathVariable String buildJobId) { | ||
| log.debug("REST request to get build job by id {}", buildJobId); | ||
|
|
||
| // Check running jobs first | ||
| BuildJobQueueItem processingJob = distributedDataAccessService.getDistributedProcessingJobs().get(buildJobId); | ||
| if (processingJob != null) { | ||
| return ResponseEntity.ok(processingJob); | ||
| } | ||
|
|
||
| // Check queued jobs | ||
| for (BuildJobQueueItem queuedJob : distributedDataAccessService.getQueuedJobs()) { | ||
| if (buildJobId.equals(queuedJob.id())) { | ||
| return ResponseEntity.ok(queuedJob); | ||
| } | ||
| } | ||
|
|
||
| // Check finished jobs in database | ||
| return buildJobRepository.findByBuildJobId(buildJobId).map(buildJob -> ResponseEntity.ok((Object) FinishedBuildJobDTO.of(buildJob))) | ||
| .orElseGet(() -> ResponseEntity.notFound().build()); |
There was a problem hiding this comment.
Validate and normalize buildJobId before lookup.
buildJobId is used directly in logging and lookups; a blank/whitespace value can cause unnecessary scans and log noise. Add a simple trim/blank guard and use the normalized value for lookups.
🔧 Suggested fix
`@GetMapping`("build-job/{buildJobId}")
public ResponseEntity<?> getBuildJobById(`@PathVariable` String buildJobId) {
- log.debug("REST request to get build job by id {}", buildJobId);
+ String normalizedBuildJobId = buildJobId == null ? null : buildJobId.trim();
+ if (normalizedBuildJobId == null || normalizedBuildJobId.isBlank()) {
+ return ResponseEntity.badRequest().build();
+ }
+ log.debug("REST request to get build job by id {}", normalizedBuildJobId);
// Check running jobs first
- BuildJobQueueItem processingJob = distributedDataAccessService.getDistributedProcessingJobs().get(buildJobId);
+ BuildJobQueueItem processingJob = distributedDataAccessService.getDistributedProcessingJobs().get(normalizedBuildJobId);
if (processingJob != null) {
return ResponseEntity.ok(processingJob);
}
@@
- for (BuildJobQueueItem queuedJob : distributedDataAccessService.getQueuedJobs()) {
- if (buildJobId.equals(queuedJob.id())) {
+ for (BuildJobQueueItem queuedJob : distributedDataAccessService.getQueuedJobs()) {
+ if (normalizedBuildJobId.equals(queuedJob.id())) {
return ResponseEntity.ok(queuedJob);
}
}
// Check finished jobs in database
- return buildJobRepository.findByBuildJobId(buildJobId).map(buildJob -> ResponseEntity.ok((Object) FinishedBuildJobDTO.of(buildJob)))
+ return buildJobRepository.findByBuildJobId(normalizedBuildJobId).map(buildJob -> ResponseEntity.ok((Object) FinishedBuildJobDTO.of(buildJob)))
.orElseGet(() -> ResponseEntity.notFound().build());
}Based on learnings "Always validate and sanitize inputs in server-side Java code. Do not trust client-side UI controls to enforce data integrity. Implement defensive checks in import/processing paths (e.g., during ExerciseImportService imports) to neutralize or normalize unexpected data, such as setting invalid exampleSolutionPublicationDate to null or applying strict nullability/format checks before persistence. Treat user input as potentially unsafe and codify input validation, normalization, and error handling as part of the core request handling and service layers."
🤖 Prompt for AI Agents
In
`@src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java`
around lines 70 - 88, In getBuildJobById, trim and validate the incoming
buildJobId up front (e.g., String normalizedId = buildJobId == null ? "" :
buildJobId.trim()), and if normalizedId.isBlank() return
ResponseEntity.badRequest().build(); then use normalizedId (not the raw
parameter) in the log statement and all lookups
(distributedDataAccessService.getDistributedProcessingJobs().get(normalizedId),
queuedJob.id().equals(normalizedId), and
buildJobRepository.findByBuildJobId(normalizedId)) and when creating the
FinishedBuildJobDTO so you never perform scans or log noise on blank/whitespace
IDs.
| @GetMapping("courses/{courseId}/build-job/{buildJobId}") | ||
| @EnforceAtLeastInstructorInCourse | ||
| public ResponseEntity<?> getBuildJobById(@PathVariable long courseId, @PathVariable String buildJobId) { | ||
| log.debug("REST request to get build job by id {} for course {}", buildJobId, courseId); | ||
|
|
||
| // Check running jobs first | ||
| BuildJobQueueItem processingJob = distributedDataAccessService.getDistributedProcessingJobs().get(buildJobId); | ||
| if (processingJob != null && processingJob.courseId() == courseId) { | ||
| return ResponseEntity.ok(processingJob); | ||
| } | ||
|
|
||
| // Check queued jobs | ||
| for (BuildJobQueueItem queuedJob : distributedDataAccessService.getQueuedJobs()) { | ||
| if (buildJobId.equals(queuedJob.id()) && queuedJob.courseId() == courseId) { | ||
| return ResponseEntity.ok(queuedJob); | ||
| } | ||
| } | ||
|
|
||
| // Check finished jobs in database | ||
| return buildJobRepository.findByBuildJobId(buildJobId).filter(buildJob -> buildJob.getCourseId() == courseId) | ||
| .map(buildJob -> ResponseEntity.ok((Object) FinishedBuildJobDTO.of(buildJob))).orElseGet(() -> ResponseEntity.notFound().build()); | ||
| } |
There was a problem hiding this comment.
Validate courseId and normalize buildJobId up front.
Guarding against non-positive course IDs and blank job IDs avoids unnecessary scans and ensures consistent ID matching.
🔧 Suggested fix
`@GetMapping`("courses/{courseId}/build-job/{buildJobId}")
`@EnforceAtLeastInstructorInCourse`
public ResponseEntity<?> getBuildJobById(`@PathVariable` long courseId, `@PathVariable` String buildJobId) {
- log.debug("REST request to get build job by id {} for course {}", buildJobId, courseId);
+ String normalizedBuildJobId = buildJobId == null ? null : buildJobId.trim();
+ if (courseId <= 0 || normalizedBuildJobId == null || normalizedBuildJobId.isBlank()) {
+ return ResponseEntity.badRequest().build();
+ }
+ log.debug("REST request to get build job by id {} for course {}", normalizedBuildJobId, courseId);
// Check running jobs first
- BuildJobQueueItem processingJob = distributedDataAccessService.getDistributedProcessingJobs().get(buildJobId);
+ BuildJobQueueItem processingJob = distributedDataAccessService.getDistributedProcessingJobs().get(normalizedBuildJobId);
if (processingJob != null && processingJob.courseId() == courseId) {
return ResponseEntity.ok(processingJob);
}
@@
- for (BuildJobQueueItem queuedJob : distributedDataAccessService.getQueuedJobs()) {
- if (buildJobId.equals(queuedJob.id()) && queuedJob.courseId() == courseId) {
+ for (BuildJobQueueItem queuedJob : distributedDataAccessService.getQueuedJobs()) {
+ if (normalizedBuildJobId.equals(queuedJob.id()) && queuedJob.courseId() == courseId) {
return ResponseEntity.ok(queuedJob);
}
}
// Check finished jobs in database
- return buildJobRepository.findByBuildJobId(buildJobId).filter(buildJob -> buildJob.getCourseId() == courseId)
+ return buildJobRepository.findByBuildJobId(normalizedBuildJobId).filter(buildJob -> buildJob.getCourseId() == courseId)
.map(buildJob -> ResponseEntity.ok((Object) FinishedBuildJobDTO.of(buildJob))).orElseGet(() -> ResponseEntity.notFound().build());
}Based on learnings "Always validate and sanitize inputs in server-side Java code. Do not trust client-side UI controls to enforce data integrity. Implement defensive checks in import/processing paths (e.g., during ExerciseImportService imports) to neutralize or normalize unexpected data, such as setting invalid exampleSolutionPublicationDate to null or applying strict nullability/format checks before persistence. Treat user input as potentially unsafe and codify input validation, normalization, and error handling as part of the core request handling and service layers."
🤖 Prompt for AI Agents
In
`@src/main/java/de/tum/cit/aet/artemis/programming/web/localci/BuildJobQueueResource.java`
around lines 78 - 99, In getBuildJobById (BuildJobQueueResource) validate the
inputs up front: ensure courseId is positive and buildJobId is not null/blank;
normalize buildJobId (e.g., trim and apply any canonical casing used elsewhere)
before any lookups; if validation fails return
ResponseEntity.badRequest().build() to avoid scanning
distributedDataAccessService.getDistributedProcessingJobs(), getQueuedJobs(), or
buildJobRepository with invalid values; then use the normalized buildJobId for
comparisons against BuildJobQueueItem.id()/courseId() and for the repository
query and mapping to FinishedBuildJobDTO.
| /** Whether the build agent was not found (offline/removed) */ | ||
| agentNotFound = signal(false); | ||
|
|
There was a problem hiding this comment.
Reset the not‑found state and distinguish 404 vs other errors.
Right now any error is treated as “not found” and agentNotFound is never cleared, which can show stale warnings and mask real failures. Consider resetting the flag on success and only using the not‑found flow for 404 responses (and clearing stale agent data).
🛠️ Suggested fix
- this.agentDetailsSubscription = this.buildAgentsService.getBuildAgentDetails(this.agentName).subscribe({
+ this.agentNotFound.set(false);
+ this.agentDetailsSubscription = this.buildAgentsService.getBuildAgentDetails(this.agentName).subscribe({
next: (buildAgent) => {
+ this.agentNotFound.set(false);
this.updateBuildAgent(buildAgent);
// Initialize filter with this agent's address to show only its finished jobs
this.finishedBuildJobFilter = new FinishedBuildJobFilter(this.buildAgent()?.buildAgent?.memberAddress);
this.loadFinishedBuildJobs();
},
- error: () => {
- this.agentNotFound.set(true);
+ error: (error: HttpErrorResponse) => {
+ if (error.status !== 404) {
+ onError(this.alertService, error);
+ return;
+ }
+ this.agentNotFound.set(true);
+ this.buildAgent.set(undefined);
// Still load finished jobs filtered by agent name
this.finishedBuildJobFilter = new FinishedBuildJobFilter(this.agentName);
this.loadFinishedBuildJobs();
},
});Also applies to: 260-272
🤖 Prompt for AI Agents
In
`@src/main/webapp/app/buildagent/build-agent-details/build-agent-details.component.ts`
around lines 82 - 84, The agentNotFound signal is currently set for all errors
and never cleared; update the fetch/load logic that reads the agent (the method
that sets agentNotFound, referenced by agentNotFound signal) so that on a
successful response you explicitly reset agentNotFound to false, and on error
only set agentNotFound to true when the HTTP status is 404 while handling
non-404 errors separately (do not set agentNotFound for other failures);
additionally, when a 404 occurs clear any stale agent data/state so the UI does
not show old agent info; apply the same changes to the other fetch path noted
around the block at lines 260-272 which uses the same agentNotFound signal.
| this.agentNotFound.set(true); | ||
| // Still load finished jobs filtered by agent name | ||
| this.finishedBuildJobFilter = new FinishedBuildJobFilter(this.agentName); | ||
| this.loadFinishedBuildJobs(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the FinishedBuildJobFilter class definition
find . -type f -name "*.ts" -o -name "*.java" | xargs grep -l "FinishedBuildJobFilter" | grep -v node_modules | head -20Repository: ls1intum/Artemis
Length of output: 771
🏁 Script executed:
# Search for buildAgentAddress and buildAgentName in TypeScript/JavaScript
rg -n "buildAgentAddress|buildAgentName" --type ts --type js -g '!node_modules' | head -40Repository: ls1intum/Artemis
Length of output: 6809
🏁 Script executed:
# Look at the component file to understand context
cat -n src/main/webapp/app/buildagent/build-agent-details/build-agent-details.component.ts | sed -n '260,280p'Repository: ls1intum/Artemis
Length of output: 1294
🏁 Script executed:
# Look at the beginning of the build-agent-details component to see what agentName is
cat -n src/main/webapp/app/buildagent/build-agent-details/build-agent-details.component.ts | head -80Repository: ls1intum/Artemis
Length of output: 5219
🏁 Script executed:
# Examine FinishedBuildJobFilter class more completely
cat -n src/main/webapp/app/buildagent/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts | sed -n '1,50p'Repository: ls1intum/Artemis
Length of output: 2788
🏁 Script executed:
# Look for backend handler of buildAgentAddress parameter
find . -type f -name "*.java" | xargs grep -l "buildAgentAddress" | grep -v test | head -10Repository: ls1intum/Artemis
Length of output: 591
🏁 Script executed:
# Look at AdminBuildJobQueueResource to find the endpoint
cat -n src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java | grep -A 20 "buildAgentAddress" | head -40Repository: ls1intum/Artemis
Length of output: 1527
🏁 Script executed:
# Search for the endpoint that fetches finished build jobs
rg -n "getFinishedBuildJobs|finished.*build.*job" src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java -A 5 | head -50Repository: ls1intum/Artemis
Length of output: 894
🏁 Script executed:
# Check BuildJobRepository for the filter logic
rg -n "buildAgentAddress" src/main/java/de/tum/cit/aet/artemis/programming/repository/BuildJobRepository.java -B 2 -A 2Repository: ls1intum/Artemis
Length of output: 973
🏁 Script executed:
# Look for where the actual filtering happens
cat -n src/main/java/de/tum/cit/aet/artemis/core/dto/pageablesearch/FinishedBuildJobPageableSearchDTO.javaRepository: ls1intum/Artemis
Length of output: 661
🏁 Script executed:
# Look at component initialization - where does agentName come from?
rg -n "agentName" src/main/webapp/app/buildagent/build-agent-details/build-agent-details.component.ts | head -20Repository: ls1intum/Artemis
Length of output: 726
🏁 Script executed:
# Check the route configuration to see what parameter is agentName
cat -n src/main/webapp/app/buildagent/build-agent-details/build-agent-details.component.ts | sed -n '80,150p'Repository: ls1intum/Artemis
Length of output: 3180
Pass agent's address, not name, to FinishedBuildJobFilter in error path.
Line 270 passes this.agentName to FinishedBuildJobFilter, but the backend filters by exact match on buildAgentAddress (see BuildJobRepository.findFinishedIdsByFilterCriteria). The success path correctly uses this.buildAgent()?.buildAgent?.memberAddress, which is the actual address. Passing a name instead will return no results. Either use the address if available in the error path, or reconsider the fallback strategy.
🤖 Prompt for AI Agents
In
`@src/main/webapp/app/buildagent/build-agent-details/build-agent-details.component.ts`
around lines 268 - 271, The error path is passing this.agentName to
FinishedBuildJobFilter which the backend filters by buildAgentAddress; update
the error path so FinishedBuildJobFilter is constructed with the agent address
instead of the name — use this.buildAgent()?.buildAgent?.memberAddress (the same
value used in the success path) when available, then call
loadFinishedBuildJobs(); keep agentNotFound.set(true) as is and only fall back
to this.agentName if no address is present.
| <input type="radio" name="options" id="option1" (click)="onTabChange(SpanType.DAY)" /> | ||
| <span jhiTranslate="artemisApp.buildQueue.statistics.daySpan"></span> | ||
| </label> | ||
| <label class="btn btn-secondary" [class.active]="currentSpan === SpanType.WEEK"> | ||
| <input type="radio" name="options" id="option2" checked (click)="onTabChange(SpanType.WEEK)" /> | ||
| <span jhiTranslate="artemisApp.buildQueue.statistics.weekSpan"></span> | ||
| </label> | ||
| <label class="btn btn-secondary" [class.active]="currentSpan === SpanType.MONTH"> | ||
| <input type="radio" name="options" id="option3" (click)="onTabChange(SpanType.MONTH)" /> | ||
| <span jhiTranslate="artemisApp.buildQueue.statistics.monthSpan"></span> |
There was a problem hiding this comment.
Avoid static radio ids to prevent duplicate IDs.
If this component renders more than once on a page, option1/2/3 will duplicate and violate HTML ID uniqueness. Since labels don’t reference them, you can drop the id attributes or generate a unique prefix.
🧹 Simple fix (remove IDs)
- <input type="radio" name="options" id="option1" (click)="onTabChange(SpanType.DAY)" />
+ <input type="radio" name="options" (click)="onTabChange(SpanType.DAY)" />
...
- <input type="radio" name="options" id="option2" checked (click)="onTabChange(SpanType.WEEK)" />
+ <input type="radio" name="options" checked (click)="onTabChange(SpanType.WEEK)" />
...
- <input type="radio" name="options" id="option3" (click)="onTabChange(SpanType.MONTH)" />
+ <input type="radio" name="options" (click)="onTabChange(SpanType.MONTH)" />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <input type="radio" name="options" id="option1" (click)="onTabChange(SpanType.DAY)" /> | |
| <span jhiTranslate="artemisApp.buildQueue.statistics.daySpan"></span> | |
| </label> | |
| <label class="btn btn-secondary" [class.active]="currentSpan === SpanType.WEEK"> | |
| <input type="radio" name="options" id="option2" checked (click)="onTabChange(SpanType.WEEK)" /> | |
| <span jhiTranslate="artemisApp.buildQueue.statistics.weekSpan"></span> | |
| </label> | |
| <label class="btn btn-secondary" [class.active]="currentSpan === SpanType.MONTH"> | |
| <input type="radio" name="options" id="option3" (click)="onTabChange(SpanType.MONTH)" /> | |
| <span jhiTranslate="artemisApp.buildQueue.statistics.monthSpan"></span> | |
| <input type="radio" name="options" (click)="onTabChange(SpanType.DAY)" /> | |
| <span jhiTranslate="artemisApp.buildQueue.statistics.daySpan"></span> | |
| </label> | |
| <label class="btn btn-secondary" [class.active]="currentSpan === SpanType.WEEK"> | |
| <input type="radio" name="options" checked (click)="onTabChange(SpanType.WEEK)" /> | |
| <span jhiTranslate="artemisApp.buildQueue.statistics.weekSpan"></span> | |
| </label> | |
| <label class="btn btn-secondary" [class.active]="currentSpan === SpanType.MONTH"> | |
| <input type="radio" name="options" (click)="onTabChange(SpanType.MONTH)" /> | |
| <span jhiTranslate="artemisApp.buildQueue.statistics.monthSpan"></span> |
🤖 Prompt for AI Agents
In
`@src/main/webapp/app/buildagent/build-job-statistics/build-job-statistics.component.html`
around lines 8 - 17, The radio inputs in the build job statistics template use
static ids ("option1"/"option2"/"option3") which can duplicate if the component
is rendered multiple times; update the template around the inputs that call
onTabChange(SpanType.DAY/WEEK/MONTH) and reference currentSpan to either remove
the id attributes entirely (since labels are not using for=) or generate unique
ids per component instance (e.g., append a component-provided unique prefix),
ensuring no duplicate HTML ids remain.
| <ngx-charts-pie-chart class="chart-container" [view]="[80, 80]" [results]="pieChartData()" [scheme]="pieChartColorScheme" [doughnut]="true" [arcWidth]="0.4" /> | ||
| <div class="d-flex flex-wrap gap-2 stats-container"> | ||
| <div class="stats-item"> | ||
| <span class="bold" jhiTranslate="artemisApp.buildQueue.statistics.totalBuilds"></span> | ||
| <span>{{ buildJobStatistics().totalBuilds }}</span> | ||
| </div> | ||
| <div class="stats-item"> | ||
| <span class="bold" jhiTranslate="artemisApp.buildQueue.statistics.successfulBuildsWithColon"></span> | ||
| <span>{{ buildJobStatistics().successfulBuilds }}</span> | ||
| <span class="text-muted small">({{ successfulBuildsPercentage() }})</span> | ||
| </div> | ||
| <div class="stats-item"> | ||
| <span class="bold" jhiTranslate="artemisApp.buildQueue.statistics.failedBuilds"></span> | ||
| <span>{{ buildJobStatistics().failedBuilds }}</span> | ||
| <span class="text-muted small">({{ failedBuildsPercentage() }})</span> | ||
| </div> | ||
| <div> | ||
| <ngx-charts-pie-chart [view]="[100, 100]" [results]="pieChartData()" [scheme]="pieChartColorScheme" [doughnut]="true" /> | ||
| <div class="stats-item"> | ||
| <span class="bold" jhiTranslate="artemisApp.buildQueue.statistics.cancelledBuilds"></span> | ||
| <span>{{ buildJobStatistics().cancelledBuilds }}</span> | ||
| <span class="text-muted small">({{ cancelledBuildsPercentage() }})</span> | ||
| </div> | ||
| <div class="stats-item"> | ||
| <span class="bold" jhiTranslate="artemisApp.buildQueue.statistics.timeoutBuilds"></span> | ||
| <span>{{ buildJobStatistics().timeOutBuilds }}</span> | ||
| <span class="text-muted small">({{ timeoutBuildsPercentage() }})</span> | ||
| </div> | ||
| @if (displayMissingBuilds) { | ||
| <div class="stats-item"> | ||
| <span class="bold" jhiTranslate="artemisApp.buildQueue.statistics.missingBuilds"></span> | ||
| <span>{{ buildJobStatistics().missingBuilds }}</span> | ||
| <span class="text-muted small">({{ missingBuildsPercentage() }})</span> | ||
| </div> | ||
| } |
There was a problem hiding this comment.
Hide “Missing” in the chart when displayMissingBuilds is false.
The chart always renders the “Missing” slice because pieChartData() includes it, even when the stats item is hidden. That’s a small UX inconsistency for embedded contexts.
Consider filtering the missing slice when displayMissingBuilds is false (e.g., in updateDisplayedBuildJobStatistics).
💡 Example adjustment (TypeScript)
- this.pieChartData.set([
- { name: 'Successful', value: statistics.successfulBuilds },
- { name: 'Failed', value: statistics.failedBuilds },
- { name: 'Cancelled', value: statistics.cancelledBuilds },
- { name: 'Timeout', value: statistics.timeOutBuilds },
- { name: 'Missing', value: statistics.missingBuilds },
- ]);
+ const data: NgxChartsSingleSeriesDataEntry[] = [
+ { name: 'Successful', value: statistics.successfulBuilds },
+ { name: 'Failed', value: statistics.failedBuilds },
+ { name: 'Cancelled', value: statistics.cancelledBuilds },
+ { name: 'Timeout', value: statistics.timeOutBuilds },
+ ];
+ if (this.displayMissingBuilds) {
+ data.push({ name: 'Missing', value: statistics.missingBuilds });
+ }
+ this.pieChartData.set(data);🤖 Prompt for AI Agents
In
`@src/main/webapp/app/buildagent/build-job-statistics/build-job-statistics.component.html`
around lines 23 - 55, The pie chart still shows the "Missing" slice because
pieChartData() always includes it; update the logic that builds chart data
(e.g., pieChartData() or updateDisplayedBuildJobStatistics()) to filter out or
omit the missing-builds entry when displayMissingBuilds is false so the chart
and the stats list stay consistent; locate where pieChartData() assembles the
array (and where buildJobStatistics()/displayMissingBuilds are referenced) and
conditionally exclude the object for missingBuilds or rebuild the chart data
from the displayedBuildJobStatistics only when displayMissingBuilds is false.
| @if (typeof buildDuration() === 'number') { | ||
| {{ buildDuration() | artemisDurationFromSeconds }} | ||
| } @else { | ||
| {{ buildDuration() }} | ||
| } | ||
| </dd> |
There was a problem hiding this comment.
typeof operator is not available in Angular templates.
The expression typeof buildDuration() === 'number' will not work in Angular templates because typeof is a JavaScript operator not exposed to template expressions. This will cause a runtime error.
Proposed fix
Create a helper method in the component or use a different approach:
- `@if` (typeof buildDuration() === 'number') {
+ `@if` (isBuildDurationNumeric()) {
{{ buildDuration() | artemisDurationFromSeconds }}
} `@else` {
{{ buildDuration() }}
}Then add to the component:
isBuildDurationNumeric(): boolean {
return typeof this.buildDuration() === 'number';
}Alternatively, refactor buildDuration() to always return consistent types.
🤖 Prompt for AI Agents
In
`@src/main/webapp/app/buildagent/build-queue/build-job-detail/build-job-detail.component.html`
around lines 214 - 219, The template uses the JS typeof operator (typeof
buildDuration() === 'number') which is not supported in Angular templates;
update the component to expose a helper boolean method (e.g.
isBuildDurationNumeric()) that returns typeof this.buildDuration() === 'number'
or refactor buildDuration() to always return a consistent type, then replace the
typeof expression in build-job-detail.component.html with a call to the helper
(isBuildDurationNumeric()) so the template condition works without using typeof.
| <ngx-datatable-column prop="buildAgent.displayName" [minWidth]="150" [width]="200"> | ||
| <ng-template ngx-datatable-header-template> | ||
| <span class="datatable-header-cell-wrapper" (click)="controls.onSort('courseId')"> | ||
| <span class="datatable-header-cell-label bold sortable" jhiTranslate="artemisApp.buildQueue.buildJob.courseId"></span> | ||
| <fa-icon [icon]="controls.iconForSortPropField('courseId')" /> | ||
| <span class="datatable-header-cell-wrapper" (click)="controls.onSort('buildAgent.displayName')"> | ||
| <span class="datatable-header-cell-label bold sortable" jhiTranslate="artemisApp.buildQueue.buildJob.buildAgent"></span> | ||
| <fa-icon [icon]="controls.iconForSortPropField('buildAgent.displayName')" /> | ||
| </span> | ||
| </ng-template> | ||
| <ng-template ngx-datatable-cell-template let-value="value" let-row="row"> | ||
| <a [routerLink]="['/admin/build-agents/details']" [queryParams]="{ agentName: row.buildAgent!.name }"> | ||
| {{ value }} | ||
| </a> | ||
| </ng-template> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the component file and check its implementation
find . -name "build-overview.component.ts" -type fRepository: ls1intum/Artemis
Length of output: 132
🏁 Script executed:
# Search for route guards and admin checks
rg -n "canActivate|ADMIN|AUTHORITY" --type=ts | head -30Repository: ls1intum/Artemis
Length of output: 3433
🏁 Script executed:
# Find routing configuration for build-agents
rg -n "/admin/build-agents|build-agents.*details" --type=tsRepository: ls1intum/Artemis
Length of output: 1016
🏁 Script executed:
# Check the component file to see if isAdministrationView exists
find . -name "build-overview.component.ts" -exec cat {} \;Repository: ls1intum/Artemis
Length of output: 22392
🏁 Script executed:
# Check the admin routes configuration for guards
cat -n src/main/webapp/app/core/admin/admin.routes.ts | head -150Repository: ls1intum/Artemis
Length of output: 7123
🏁 Script executed:
# Search for UserRouteAccessService and how admin routes are protected
rg -A 3 "canActivate.*UserRouteAccessService|Authority\\.ADMIN" --type=ts | head -50Repository: ls1intum/Artemis
Length of output: 4030
🏁 Script executed:
# Check if there's a specific admin route guard
rg -n "build-agents" --type=ts -B 2 -A 2Repository: ls1intum/Artemis
Length of output: 10960
🏁 Script executed:
# Check the LocalCIGuard implementation to understand what it protects
cat -n src/main/webapp/app/buildagent/shared/localci-guard.service.tsRepository: ls1intum/Artemis
Length of output: 855
🏁 Script executed:
# Check the template to see how it currently handles the link
cat -n src/main/webapp/app/buildagent/build-queue/build-overview.component.html | sed -n '90,110p'Repository: ls1intum/Artemis
Length of output: 1855
Use modern control flow syntax and conditionally render admin link based on view mode
The template links to /admin/build-agents/details unconditionally, but course-level users shouldn't see this link when viewing course-scoped build queues (courseId > 0). Wrap the link with @if (isAdministrationView()) and render plain text for non-admin contexts. Also update to use modern Angular control-flow syntax (@if) instead of legacy directives.
🔧 Suggested adjustment (modern control flow)
<ng-template ngx-datatable-cell-template let-value="value" let-row="row">
- <a [routerLink]="['/admin/build-agents/details']" [queryParams]="{ agentName: row.buildAgent!.name }">
- {{ value }}
- </a>
+ `@if` (isAdministrationView()) {
+ <a [routerLink]="['/admin/build-agents/details']" [queryParams]="{ agentName: row.buildAgent!.name }">
+ {{ value }}
+ </a>
+ } `@else` {
+ <span>{{ value }}</span>
+ }
</ng-template>🤖 Prompt for AI Agents
In `@src/main/webapp/app/buildagent/build-queue/build-overview.component.html`
around lines 95 - 106, The agent link to admin details is rendered
unconditionally; change the cell template in build-overview.component.html to
conditionally render the <a> only when isAdministrationView() returns true and
render plain text otherwise (use Angular's *ngIf with an else template or the
short-circuit pattern). Locate the ngx-datatable-cell-template (where value and
row are available) and wrap the current routerLink anchor with
*ngIf="isAdministrationView()", moving the current display into that branch, and
add an else template (e.g., plainAgent) that outputs {{ value }} as plain text;
keep the existing bindings (row.buildAgent!.name and value) and the header sort
handlers (controls.onSort, controls.iconForSortPropField) untouched.
| <ngx-datatable-column prop="cancel" [minWidth]="150" [width]="100"> | ||
| <div class="d-flex justify-content-center align-items-center"> | ||
| <ng-template ngx-datatable-header-template> | ||
| <span class="datatable-header-cell-wrapper"> | ||
| <button class="btn btn-danger btn-sm" (click)="cancelAllRunningBuildJobs()"> | ||
| <fa-icon [icon]="faTimes" /> | ||
| <span jhiTranslate="artemisApp.buildQueue.cancelAll"></span> | ||
| </button> | ||
| </span> | ||
| </ng-template> | ||
| <ng-template ngx-datatable-cell-template let-row="row"> | ||
| <button class="btn btn-danger btn-sm" (click)="cancelBuildJob(row.id)"> | ||
| <fa-icon [icon]="faTimes" /> | ||
| <span jhiTranslate="artemisApp.buildQueue.cancelAll"></span> | ||
| </button> | ||
| </ng-template> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
WCAG 2.1 accessible name icon-only buttons aria-label requirements
💡 Result:
- WCAG 2.1 doesn’t require
aria-labelspecifically for icon-only buttons; it requires that every user interface component has a programmatically determinable accessible name (and role/value as applicable). (w3.org) - For an icon-only
<button>, you must provide a non-empty accessible name, typically via one of:aria-label="Search"aria-labelledby="idOfVisibleOrHiddenText"- Real text inside the button (e.g., a visually-hidden
<span>Search</span>) (w3c.github.io)
- If the button does have a visible text label (not icon-only), then WCAG 2.5.3 (Label in Name) applies: the accessible name must contain the visible label text (so don’t replace “Save” with
aria-label="Store changes"). (w3.org) - Practical note:
aria-labelparticipates in accessible-name computation and can override other label sources, so use it carefully when visible text exists. (developer.mozilla.org)
Citations:
- 1: https://www.w3.org/WAI/WCAG21/Understanding/name-role-value?utm_source=openai
- 2: https://w3c.github.io/wcag/understanding/name-role-value.html?utm_source=openai
- 3: https://www.w3.org/WAI/WCAG21/Understanding/label-in-name.html?utm_source=openai
- 4: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-label?utm_source=openai
🏁 Script executed:
# First, let's check if the file exists and examine the specific line ranges mentioned
wc -l src/main/webapp/app/buildagent/build-queue/build-overview.component.htmlRepository: ls1intum/Artemis
Length of output: 136
🏁 Script executed:
# Check lines 271-285 (shown in review)
sed -n '271,285p' src/main/webapp/app/buildagent/build-queue/build-overview.component.html | cat -nRepository: ls1intum/Artemis
Length of output: 1358
🏁 Script executed:
# Check lines 455-468 (mentioned as also applies to)
sed -n '455,468p' src/main/webapp/app/buildagent/build-queue/build-overview.component.html | cat -nRepository: ls1intum/Artemis
Length of output: 1299
🏁 Script executed:
# Check lines 613-617 (mentioned as also applies to)
sed -n '613,617p' src/main/webapp/app/buildagent/build-queue/build-overview.component.html | cat -nRepository: ls1intum/Artemis
Length of output: 454
🏁 Script executed:
# Search for the view logs <a> element mentioned in the diff
grep -n "viewBuildLogs\|view.*[Ll]ogs" src/main/webapp/app/buildagent/build-queue/build-overview.component.html | head -20Repository: ls1intum/Artemis
Length of output: 286
🏁 Script executed:
# Check broader context around line 615 (the viewBuildLogs <a>)
sed -n '610,620p' src/main/webapp/app/buildagent/build-queue/build-overview.component.html | cat -nRepository: ls1intum/Artemis
Length of output: 856
🏁 Script executed:
# Look for all <a> elements with viewBuildLogs or detail-link
grep -B2 -A2 "detail-link\|viewBuildLogs" src/main/webapp/app/buildagent/build-queue/build-overview.component.htmlRepository: ls1intum/Artemis
Length of output: 491
🏁 Script executed:
# Check broader context around lines 455-468 to see if there's another <a> element
sed -n '450,470p' src/main/webapp/app/buildagent/build-queue/build-overview.component.html | cat -nRepository: ls1intum/Artemis
Length of output: 1737
Add accessible names for icon-only buttons and convert <a> elements without href to buttons
Icon-only buttons (showing only the faTimes icon with no text) lack accessible names required by WCAG 2.1. Add aria-label to those buttons. However, the header "Cancel All" buttons already have visible text via the <span jhiTranslate> element and do not need additional aria-label attributes.
The <a> element at line 613-617 used as a button (via click handler) lacks an href attribute, making it not keyboard-focusable. Convert it to a <button> element.
Specific fixes needed
- Lines 271-285 (row action button): Add
aria-label="Cancel build job"to the icon-only cancel button inside thengx-datatable-cell-template - Lines 455-468 (row action button): Add
aria-labelto the icon-only cancel button - Lines 613-617 (
<a>element): Convert to<button type="button">with proper button semantics
Note: The header cancel buttons with visible text labels already have accessible names and should not receive aria-label attributes to maintain WCAG 2.5.3 (Label in Name) compliance.
🤖 Prompt for AI Agents
In `@src/main/webapp/app/buildagent/build-queue/build-overview.component.html`
around lines 271 - 285, The icon-only cancel buttons inside the
ngx-datatable-cell-template need accessible names: add aria-label="Cancel build
job" to the button that calls cancelBuildJob(row.id) (the element showing
<fa-icon [icon]="faTimes" />) while leaving the header cancel button (which
calls cancelAllRunningBuildJobs() and already contains visible text) unchanged;
additionally, replace the <a> element used as a click-only control with a proper
<button type="button"> to ensure keyboard focus and correct semantics.
src/main/webapp/app/buildagent/build-queue/build-overview.component.ts
Outdated
Show resolved
Hide resolved
- Change URL from build-queue to build-overview - Change URL pattern from query params to path params (:jobId/job-details) - Update all navigation links and sidebar items - Improve finished builds table styling: - Smaller font for name, duration, and completion date - Add time (HH:mm:ss) to completion date - Truncate name to 25 characters with ellipsis - Adjust column widths Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Apply 0.75rem font size to all cells in clickable rows - Remove individual compact-cell classes Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/main/webapp/app/buildagent/build-queue/build-job-detail/build-job-detail.component.ts`:
- Around line 215-220: The catch is currently empty and swallows errors from
downloadFile(this.buildJobId), so change it to catch the error (e.g., catch
(err)) and surface it — call this.alertService.error with the original error
message or include the error object (e.g., pass err.message or err) and also log
it via console.error for debugging; update the try/catch around
downloadFile(blob, `${this.buildJobId}.log`) to use the caught error when
reporting.
🧹 Nitpick comments (4)
src/main/webapp/app/buildagent/build-queue/build-job-detail/build-job-detail.component.html (1)
180-183: Consider simplifying the retry count condition.The current check calls
getRetryCount()twice and uses a non-null assertion. SincegetRetryCount()returnsnumber | undefined, you could simplify this:Suggested simplification
- `@if` (getRetryCount() !== undefined && getRetryCount()! > 0) { + `@if` ((getRetryCount() ?? 0) > 0) { <dt jhiTranslate="artemisApp.buildQueue.detail.retryCount"></dt> <dd>{{ getRetryCount() }}</dd> }src/main/webapp/app/buildagent/build-queue/build-job-detail/build-job-detail.component.ts (3)
45-46: Avoid usinganytype for the build job signal.Per coding guidelines requiring 100% type safety,
signal<any | undefined>loses all type checking. Consider using a proper union type or the actual model types.Suggested improvement
+import { FinishedBuildJob } from 'app/buildagent/shared/entities/finished-build-job.model'; + +// Define a union type for the different job states +type BuildJobData = BuildJob | FinishedBuildJob; + /** The build job data - either a BuildJob (queued/running) or FinishedBuildJob */ -buildJob = signal<any | undefined>(undefined); +buildJob = signal<BuildJobData | undefined>(undefined);This would also require updating the helper methods to handle the union type properly, but provides compile-time type safety.
97-113:buildDurationreturns inconsistent types causing template issues.The computed signal returns either a string (e.g.,
"1.234s"), a number, orundefined. This mixed return type causes thetypeofcheck issue in the template and makes the code harder to reason about.Suggested fix
Consider always returning a number (seconds) and let the template handle formatting:
/** Build duration in seconds */ -buildDuration = computed(() => { +buildDuration = computed((): number | undefined => { const job = this.buildJob(); if (!job) return undefined; // For finished jobs, compute from start/completion dates if (job.buildStartDate && job.buildCompletionDate) { - return (dayjs(job.buildCompletionDate).diff(dayjs(job.buildStartDate), 'milliseconds') / 1000).toFixed(3) + 's'; + return dayjs(job.buildCompletionDate).diff(dayjs(job.buildStartDate), 'milliseconds') / 1000; } // For running jobs, use jobTimingInfo.buildDuration (updated by interval) if (job.jobTimingInfo?.buildDuration) { return job.jobTimingInfo.buildDuration; } if (job.buildDuration) { return job.buildDuration; } return undefined; });Then in the template, always use the pipe:
{{ buildDuration() | artemisDurationFromSeconds }}
286-290: Helper methods returnanytype.Methods
getSubmissionDate(),getBuildStartDate(), andgetBuildCompletionDate()have return typeany. Consider using proper date types for better type safety.Suggested improvement
/** Get submission date */ -getSubmissionDate(): any { +getSubmissionDate(): dayjs.Dayjs | string | undefined { const job = this.buildJob(); if (!job) return undefined; return job.buildSubmissionDate || job.jobTimingInfo?.submissionDate; }Apply similar changes to
getBuildStartDate()andgetBuildCompletionDate().Also applies to: 293-297, 300-304
| try { | ||
| downloadFile(blob, `${this.buildJobId}.log`); | ||
| } catch { | ||
| this.alertService.error('artemisApp.buildQueue.logs.downloadError'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Empty catch block silently swallows errors.
The empty catch block hides potential issues. The downloadFile utility might throw for various reasons (e.g., browser restrictions, invalid blob).
Suggested fix
try {
downloadFile(blob, `${this.buildJobId}.log`);
- } catch {
+ } catch (error) {
+ console.error('Failed to download build logs:', error);
this.alertService.error('artemisApp.buildQueue.logs.downloadError');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| downloadFile(blob, `${this.buildJobId}.log`); | |
| } catch { | |
| this.alertService.error('artemisApp.buildQueue.logs.downloadError'); | |
| } | |
| } | |
| try { | |
| downloadFile(blob, `${this.buildJobId}.log`); | |
| } catch (error) { | |
| console.error('Failed to download build logs:', error); | |
| this.alertService.error('artemisApp.buildQueue.logs.downloadError'); | |
| } |
🤖 Prompt for AI Agents
In
`@src/main/webapp/app/buildagent/build-queue/build-job-detail/build-job-detail.component.ts`
around lines 215 - 220, The catch is currently empty and swallows errors from
downloadFile(this.buildJobId), so change it to catch the error (e.g., catch
(err)) and surface it — call this.alertService.error with the original error
message or include the error object (e.g., pass err.message or err) and also log
it via console.error for debugging; update the try/catch around
downloadFile(blob, `${this.buildJobId}.log`) to use the caught error when
reporting.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
github-actions
bot
added
the
config-change
…uality requirements Combined two debug log statements into one to meet the maximum 1000 lines threshold. Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildJobDTO.java`:
- Around line 1-46: The BuildJobDTO.status() method is annotated `@Nullable` but
FinishedBuildJobDTO implements it as non-null, violating the interface contract;
update FinishedBuildJobDTO to declare its status() return as nullable (or
explicitly annotate it with `@Nullable`) so the implementation's signature is
Liskov-compatible with BuildJobDTO.status(), ensuring polymorphic type-safety
when using BuildJobDTO references.
In
`@src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java`:
- Around line 247-269: The tryInitialize method in BuildJobManagementService
currently adds a new listener to canceledBuildJobsTopic on each successful init
which can duplicate handlers after reconnection; modify the class to store the
listener registration (e.g., a field like canceledBuildJobsListenerId or a
Disposable/Registration reference) when calling
canceledBuildJobsTopic.addMessageListener, and before adding a new listener
remove/unregister the previous one (call the topic's remove/unregister method or
dispose the previous Registration) so only a single active listener exists;
ensure this logic is used wherever initialized is reset and tryInitialize is
retried.
In
`@src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java`:
- Around line 276-313: The pause/resume topic listeners are re-registered on
reconnect causing duplicates; before calling
distributedDataAccessService.getPauseBuildAgentTopic().addMessageListener(...)
and getResumeBuildAgentTopic().addMessageListener(...) in tryInitialize(),
remove any previously-registered listeners and store the new registration IDs in
fields (e.g. pauseListenerId, resumeListenerId) so they can be removed on
subsequent re-initializations; implement null-check removal (like you do for
listenerId with getDistributedBuildJobQueue().removeListener(this.listenerId)),
assign the returned registration IDs from addMessageListener to the new fields,
and use those fields when removing listeners.
In
`@src/main/webapp/app/buildagent/build-job-statistics/build-job-statistics.component.ts`:
- Around line 192-229: The incrementStatisticsByStatus function increments
totalBuilds regardless of whether status matches a known case, causing counters
to drift; fix it by validating the status before bumping totals: either add a
default branch in the switch that logs a warning (use the component logger or
console) and returns without calling updateDisplayedBuildJobStatistics, or move
the totalBuilds increment after the switch and only add 1 when a known case was
handled; reference incrementStatisticsByStatus, BuildJobStatistics, and
updateDisplayedBuildJobStatistics to locate and change the logic accordingly.
In
`@src/main/webapp/app/buildagent/build-queue/tables/queued-jobs-table/queued-jobs-table.component.html`:
- Around line 22-24: The clickable table rows (class "clickable-row") only
respond to mouse clicks via the (click)="onJobClick(buildJob.id)" handler,
excluding keyboard users; make rows keyboard-accessible by adding tabindex="0",
role="button" (or role="row" with proper semantics), and a (keydown) handler
that detects Enter and Space to call onJobClick(buildJob.id); ensure any focus
styling and ARIA attributes (e.g., aria-label or aria-describedby if needed) are
applied so screen reader and keyboard users can activate the row just like a
mouse click.
In
`@src/main/webapp/app/buildagent/build-queue/tables/queued-jobs-table/queued-jobs-table.component.ts`:
- Around line 87-105: The calculateWaitingTime function can produce negative
outputs when diffSeconds is negative due to client/server clock skew; modify
calculateWaitingTime to detect when diffSeconds < 0 (use the existing
diffSeconds variable) and treat it as zero (e.g., return "0s" or clamp
diffSeconds to 0) before the existing formatting logic so the function never
returns negative durations.
🧹 Nitpick comments (15)
src/main/webapp/i18n/en/buildAgents.json (1)
9-10: Consider slightly clearer short labels.“Max” and “Current” can be ambiguous outside a tight table context; consider “Max concurrent” / “Current running” or a tooltip to preserve clarity.
src/main/webapp/app/buildagent/build-agent-details/build-agent-details.component.spec.ts (1)
212-212: Consider using consistent casting pattern.The double cast
as unknown as MockRouterdiffers from the pattern used foractivatedRouteon line 210 which uses a direct cast. While this works, you could align the patterns for consistency:- router = TestBed.inject(Router) as unknown as MockRouter; + router = TestBed.inject(Router) as MockRouter;If the direct cast causes a TypeScript error, the current approach is acceptable for test code.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java (1)
149-160: Consider batching WebSocket notifications for bulk cancellations.The current implementation performs a database lookup and WebSocket send for each cancelled job individually. For bulk operations like
cancelAllQueuedBuildJobs(), this could be optimized by batching the notifications.💡 Optional optimization for bulk operations
private void updateCancelledQueuedBuildJobsStatus(List<BuildJobQueueItem> queuedJobs) { + List<FinishedBuildJobDTO> finishedJobs = new ArrayList<>(); for (BuildJobQueueItem queuedJob : queuedJobs) { buildJobRepository.updateBuildJobStatus(queuedJob.id(), BuildStatus.CANCELLED); - // Send WebSocket notification for the cancelled job (only on scheduling node) - localCIQueueWebsocketService.ifPresent(websocketService -> { - buildJobRepository.findByBuildJobId(queuedJob.id()).ifPresent(buildJob -> { - FinishedBuildJobDTO finishedBuildJobDTO = FinishedBuildJobDTO.of(buildJob); - websocketService.sendFinishedBuildJobOverWebsocket(finishedBuildJobDTO); - }); - }); + localCIQueueWebsocketService.ifPresent(ws -> + buildJobRepository.findByBuildJobId(queuedJob.id()) + .map(FinishedBuildJobDTO::of) + .ifPresent(finishedJobs::add)); } + // Send batch notification + localCIQueueWebsocketService.ifPresent(ws -> + finishedJobs.forEach(ws::sendFinishedBuildJobOverWebsocket)); }src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (1)
338-349: Consider extracting WebSocket notification to a helper method.The WebSocket notification pattern (refetch → convert to DTO → send) is duplicated between this class and
SharedQueueManagementService. Consider extracting it to a shared helper or theLocalCIQueueWebsocketServiceitself.💡 Example: Move logic to LocalCIQueueWebsocketService
// In LocalCIQueueWebsocketService public void sendFinishedBuildJobNotification(String buildJobId) { buildJobRepository.findWithDataByBuildJobId(buildJobId).ifPresent(buildJob -> { FinishedBuildJobDTO dto = FinishedBuildJobDTO.of(buildJob); sendFinishedBuildJobOverWebsocket(dto); }); }Then callers can simply invoke:
localCIQueueWebsocketService.ifPresent(service -> service.sendFinishedBuildJobNotification(finalSavedBuildJob.getBuildJobId()));src/main/webapp/app/buildagent/build-queue/tables/queued-jobs-table/queued-jobs-table.component.spec.ts (1)
91-105: Minor: MissingawaitbeforeTestBed.configureTestingModule.For consistency with modern Angular testing patterns,
configureTestingModuleshould be awaited when the function is async.♻️ Suggested fix
beforeEach(async () => { - TestBed.configureTestingModule({ + await TestBed.configureTestingModule({ imports: [QueuedJobsTableComponent], providers: [ { provide: TranslateService, useClass: MockTranslateService }, { provide: ActivatedRoute, useValue: new MockActivatedRoute({}) }, ], schemas: [NO_ERRORS_SCHEMA], }); await TestBed.compileComponents();src/main/webapp/app/buildagent/build-agent-summary/build-agent-summary.component.html (1)
39-44: Consider a fallback for thetrackexpression.The
trackexpression usesagent.buildAgent?.name, which could beundefinedifbuildAgentis missing. This may cause Angular to track multiple agents with the sameundefinedkey, leading to rendering anomalies.♻️ Suggested fix
- `@for` (agent of buildAgents(); track agent.buildAgent?.name) { + `@for` (agent of buildAgents(); track agent.buildAgent?.name ?? agent.id ?? $index) {src/main/webapp/app/buildagent/build-queue/build-overview.component.html (1)
4-52: Consider extracting the duplicated navigation bar into a shared template or component.Lines 4-24 and 29-51 contain nearly identical navigation bar markup for admin and course views. This duplication increases maintenance burden.
♻️ Suggested approach using ng-template
<ng-template `#jumpToNav`> <span class="text-muted small" jhiTranslate="artemisApp.buildQueue.jumpTo"></span> <div class="btn-group btn-group-sm"> <button class="btn btn-outline-secondary" (click)="scrollToSection('build-queue-running-heading')"> <span jhiTranslate="artemisApp.buildQueue.running"></span> <span class="badge bg-primary ms-1">{{ runningBuildJobs()?.length || 0 }}</span> </button> <button class="btn btn-outline-secondary" (click)="scrollToSection('build-queue-queued-heading')"> <span jhiTranslate="artemisApp.buildQueue.queued"></span> <span class="badge bg-primary ms-1">{{ queuedBuildJobs()?.length || 0 }}</span> </button> <button class="btn btn-outline-secondary" (click)="scrollToSection('build-queue-finished-heading')"> <span jhiTranslate="artemisApp.buildQueue.finished"></span> </button> </div> <span class="text-muted small" jhiTranslate="artemisApp.buildQueue.buildCapacityInfo" [translateValues]="{ running: currentBuilds(), capacity: buildCapacity(), agents: onlineAgents() }" ></span> </ng-template>Then use
<ng-container *ngTemplateOutlet="jumpToNav"></ng-container>in both locations.src/main/webapp/app/buildagent/build-queue/tables/finished-jobs-table/finished-jobs-table.component.ts (1)
13-19: Minor inconsistency:styleUrlvsstyleUrlsacross sibling components.This component uses
styleUrl(singular) whileRunningJobsTableComponentandQueuedJobsTableComponentusestyleUrls(plural with array). Both work, but for consistency across the table suite:♻️ Suggested change for consistency
`@Component`({ selector: 'jhi-finished-jobs-table', templateUrl: './finished-jobs-table.component.html', - styleUrl: './finished-jobs-table.component.scss', + styleUrls: ['./finished-jobs-table.component.scss'], changeDetection: ChangeDetectionStrategy.OnPush,src/main/webapp/app/buildagent/build-queue/build-job-detail/build-job-detail.component.ts (3)
45-46: Prefer typed signals overanyfor type safety.Using
anyforbuildJobsignal loses TypeScript's type checking benefits. Consider using a union type or a base interface that bothBuildJobandFinishedBuildJobextend.♻️ Suggested improvement
+import { BuildJob, FinishedBuildJob } from 'app/buildagent/shared/entities/build-job.model'; + +type BuildJobData = BuildJob | FinishedBuildJob; + /** The build job data - either a BuildJob (queued/running) or FinishedBuildJob */ -buildJob = signal<any | undefined>(undefined); +buildJob = signal<BuildJobData | undefined>(undefined);Then update the helper methods to use proper type guards or optional chaining with typed properties.
As per coding guidelines: "prefer 100% type safety"
173-181: Consider using a more efficient approach for the duration update interval.The current implementation creates a new object spread on every interval tick (every second), which triggers change detection even when the job is finished or undefined.
♻️ Suggested improvement
this.durationInterval = setInterval(() => { const job = this.buildJob(); - if (job?.jobTimingInfo?.buildStartDate && !this.isFinished()) { + if (!this.isFinished() && job?.jobTimingInfo?.buildStartDate) { const start = dayjs(job.jobTimingInfo.buildStartDate); const now = dayjs(); - const updatedJob = { ...job, jobTimingInfo: { ...job.jobTimingInfo, buildDuration: now.diff(start, 'seconds') } }; - this.buildJob.set(updatedJob); + // Update in-place to avoid unnecessary object creation + job.jobTimingInfo.buildDuration = now.diff(start, 'seconds'); + this.buildJob.set({ ...job }); } }, 1000);Note: The check order was also swapped to short-circuit earlier when the job is finished.
217-228: WebSocket subscription should be established after initial load completes.Currently,
initWebsocketSubscription()is called immediately inngOnInit, potentially racing with the initial HTTP load. If a WebSocket update arrives before the HTTP response, the subsequent auto-log-loading transition check may behave unexpectedly.♻️ Suggested approach
ngOnInit() { // ... this.loadBuildJob(); - this.initWebsocketSubscription(); + // WebSocket subscription is initialized after successful loadThen call
initWebsocketSubscription()in thenextcallback ofloadBuildJob()after setting the job.src/main/java/de/tum/cit/aet/artemis/programming/service/localci/DistributedDataAccessService.java (1)
207-223: Verify the semantic difference between "empty set" and "cannot determine".The logic assumes
connectedClients.isEmpty()means the connected clients list cannot be determined (e.g., running as a client or using Redis provider). However, an empty set could also legitimately mean "no clients are currently connected." This ambiguity could cause all agents to be returned when there are actually zero connected clients.Consider having
getConnectedClientNames()returnOptional<Set<String>>whereOptional.empty()indicates "not supported" andOptional.of(emptySet)indicates "no clients connected."🔧 Alternative approach using Optional
In
DistributedDataProviderinterface and implementations:- Set<String> getConnectedClientNames(); + Optional<Set<String>> getConnectedClientNames();Then in this method:
- Set<String> connectedClients = distributedDataProvider.getConnectedClientNames(); - if (connectedClients.isEmpty()) { - return allAgents; - } - return allAgents.stream().filter(agent -> connectedClients.contains(agent.buildAgent().name())).toList(); + Optional<Set<String>> connectedClients = distributedDataProvider.getConnectedClientNames(); + if (connectedClients.isEmpty()) { + // Not supported on this provider (client mode, Redis, local) + return allAgents; + } + // Filter to connected clients only + return allAgents.stream().filter(agent -> connectedClients.get().contains(agent.buildAgent().name())).toList();src/main/java/de/tum/cit/aet/artemis/programming/service/localci/distributed/hazelcast/HazelcastDistributedDataProviderService.java (3)
353-370: Immediate callback should respect connection history.When a listener is registered after the initial connection has already occurred, the callback is always invoked with
true(line 362). However, ifhasConnectedBeforeis alreadytrue, this could be misleading since it wasn't truly the "initial" connection from this listener's perspective.Consider checking
hasConnectedBeforeto provide more accurate information:🔧 Suggested fix
// If already connected, notify the listener immediately // This handles the case where a service registers a listener after initial connection if (isConnectedToCluster()) { try { - // For late registrations after initial connection, treat as initial - callback.accept(true); + // Pass the actual connection state - if this is the first connection ever + // (hasConnectedBefore was false before this registration), it's initial. + // Otherwise, the service is registering after initial connection already happened. + callback.accept(!hasConnectedBefore.get()); } catch (Exception e) { log.error("Error invoking connection state callback on registration: {}", e.getMessage(), e); } }
55-63: Consider adding cleanup for the Hazelcast lifecycle listener.The
hazelcastLifecycleListenerIdis stored (line 58, 99) but never used to remove the listener. If this service is destroyed while the Hazelcast instance is still running, the listener remains registered.Consider implementing a
@PreDestroymethod to clean up:🔧 Suggested cleanup method
`@PreDestroy` public void cleanup() { if (hazelcastLifecycleListenerId != null && isInstanceRunning()) { hazelcastInstance.getLifecycleService().removeLifecycleListener(hazelcastLifecycleListenerId); } connectionStateListeners.clear(); }
321-334: Consider caching the cluster membership check result briefly.The
isConnectedToCluster()method callsclientProxy.getCluster().getMembers().size()which may involve network operations. If this method is called frequently (e.g., in health checks or before each distributed operation), it could add latency.The current implementation is correct and safe with the try-catch, but if performance becomes a concern, consider caching the result for a short duration.
| package de.tum.cit.aet.artemis.buildagent.dto; | ||
|
|
||
| import org.jspecify.annotations.Nullable; | ||
|
|
||
| import de.tum.cit.aet.artemis.programming.domain.build.BuildStatus; | ||
|
|
||
| /** | ||
| * Common interface for build job DTOs, providing access to fields shared between | ||
| * active jobs ({@link BuildJobQueueItem}) and finished jobs ({@link FinishedBuildJobDTO}). | ||
| * <p> | ||
| * This interface enables type-safe handling of build jobs in REST endpoints that | ||
| * may return either type depending on the job's state. | ||
| */ | ||
| public interface BuildJobDTO { | ||
|
|
||
| /** | ||
| * @return the unique identifier of the build job | ||
| */ | ||
| String id(); | ||
|
|
||
| /** | ||
| * @return the name/description of the build job | ||
| */ | ||
| String name(); | ||
|
|
||
| /** | ||
| * @return the ID of the participation this build job belongs to | ||
| */ | ||
| long participationId(); | ||
|
|
||
| /** | ||
| * @return the ID of the course this build job belongs to | ||
| */ | ||
| long courseId(); | ||
|
|
||
| /** | ||
| * @return the ID of the exercise this build job belongs to | ||
| */ | ||
| long exerciseId(); | ||
|
|
||
| /** | ||
| * @return the current status of the build job, or null if not yet determined | ||
| */ | ||
| @Nullable | ||
| BuildStatus status(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find BuildJobQueueItem and FinishedBuildJobDTO implementations
echo "=== Searching for BuildJobQueueItem ==="
rg -n "class BuildJobQueueItem|record BuildJobQueueItem" --type java -A 5
echo "=== Searching for FinishedBuildJobDTO ==="
rg -n "class FinishedBuildJobDTO|record FinishedBuildJobDTO" --type java -A 5Repository: ls1intum/Artemis
Length of output: 2092
🏁 Script executed:
#!/bin/bash
# Get complete view of FinishedBuildJobDTO
echo "=== FinishedBuildJobDTO full view ==="
cat -n src/main/java/de/tum/cit/aet/artemis/buildagent/dto/FinishedBuildJobDTO.java | head -30
echo ""
echo "=== BuildJobQueueItem full view ==="
cat -n src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildJobQueueItem.java | head -30Repository: ls1intum/Artemis
Length of output: 3638
Interface implementation is complete but has a type contract inconsistency.
Both BuildJobQueueItem and FinishedBuildJobDTO correctly implement the BuildJobDTO interface with all required methods. However, FinishedBuildJobDTO declares status() as non-nullable while the interface specifies it as @Nullable. Although semantically sensible (finished jobs always have a determined status), this violates the interface contract and may cause type safety issues when the interface is used polymorphically. Consider marking status() as @Nullable in FinishedBuildJobDTO for consistency, or document why the stricter contract is intentional.
🤖 Prompt for AI Agents
In `@src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildJobDTO.java` around
lines 1 - 46, The BuildJobDTO.status() method is annotated `@Nullable` but
FinishedBuildJobDTO implements it as non-null, violating the interface contract;
update FinishedBuildJobDTO to declare its status() return as nullable (or
explicitly annotate it with `@Nullable`) so the implementation's signature is
Liskov-compatible with BuildJobDTO.status(), ensuring polymorphic type-safety
when using BuildJobDTO references.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java
Show resolved
Hide resolved
| incrementStatisticsByStatus(status: string | undefined): void { | ||
| if (!status) { | ||
| return; | ||
| } | ||
|
|
||
| const currentStats = this.buildJobStatistics(); | ||
| const updatedStats = new BuildJobStatistics(); | ||
|
|
||
| // Copy current values | ||
| updatedStats.totalBuilds = currentStats.totalBuilds + 1; | ||
| updatedStats.successfulBuilds = currentStats.successfulBuilds; | ||
| updatedStats.failedBuilds = currentStats.failedBuilds; | ||
| updatedStats.cancelledBuilds = currentStats.cancelledBuilds; | ||
| updatedStats.timeOutBuilds = currentStats.timeOutBuilds; | ||
| updatedStats.missingBuilds = currentStats.missingBuilds; | ||
|
|
||
| // Increment the appropriate counter based on status | ||
| switch (status) { | ||
| case 'SUCCESSFUL': | ||
| updatedStats.successfulBuilds++; | ||
| break; | ||
| case 'FAILED': | ||
| case 'ERROR': | ||
| updatedStats.failedBuilds++; | ||
| break; | ||
| case 'CANCELLED': | ||
| updatedStats.cancelledBuilds++; | ||
| break; | ||
| case 'TIMEOUT': | ||
| updatedStats.timeOutBuilds++; | ||
| break; | ||
| case 'MISSING': | ||
| updatedStats.missingBuilds++; | ||
| break; | ||
| } | ||
|
|
||
| this.updateDisplayedBuildJobStatistics(updatedStats); | ||
| } |
There was a problem hiding this comment.
Unknown status values will cause statistics drift.
When status doesn't match any known case (e.g., a new status is added in the future, or an unexpected value arrives), totalBuilds is incremented but no specific counter is updated. This causes the sum of individual counters to diverge from totalBuilds over time.
Consider adding a default case to handle unknown statuses, either by logging a warning or by not incrementing totalBuilds for unrecognized values.
🛠️ Proposed fix
case 'MISSING':
updatedStats.missingBuilds++;
break;
+ default:
+ // Unknown status - don't increment totalBuilds to avoid statistics drift
+ updatedStats.totalBuilds--;
+ console.warn(`Unknown build job status: ${status}`);
+ break;
}🤖 Prompt for AI Agents
In
`@src/main/webapp/app/buildagent/build-job-statistics/build-job-statistics.component.ts`
around lines 192 - 229, The incrementStatisticsByStatus function increments
totalBuilds regardless of whether status matches a known case, causing counters
to drift; fix it by validating the status before bumping totals: either add a
default branch in the switch that logs a warning (use the component logger or
console) and returns without calling updateDisplayedBuildJobStatistics, or move
the totalBuilds increment after the switch and only add 1 when a known case was
handled; reference incrementStatisticsByStatus, BuildJobStatistics, and
updateDisplayedBuildJobStatistics to locate and change the logic accordingly.
.../webapp/app/buildagent/build-queue/tables/queued-jobs-table/queued-jobs-table.component.html
Show resolved
Hide resolved
| calculateWaitingTime(submissionDate: dayjs.Dayjs | undefined): string { | ||
| if (!submissionDate) { | ||
| return '-'; | ||
| } | ||
| const now = dayjs(); | ||
| const diffSeconds = now.diff(submissionDate, 'seconds'); | ||
|
|
||
| if (diffSeconds < 60) { | ||
| return `${diffSeconds}s`; | ||
| } else if (diffSeconds < 3600) { | ||
| const minutes = Math.floor(diffSeconds / 60); | ||
| const seconds = diffSeconds % 60; | ||
| return `${minutes}m ${seconds}s`; | ||
| } else { | ||
| const hours = Math.floor(diffSeconds / 3600); | ||
| const minutes = Math.floor((diffSeconds % 3600) / 60); | ||
| return `${hours}h ${minutes}m`; | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle potential negative time differences from clock skew.
If the client clock is ahead of the server, diffSeconds could be negative, resulting in confusing output like "-5s".
🔧 Suggested fix
calculateWaitingTime(submissionDate: dayjs.Dayjs | undefined): string {
if (!submissionDate) {
return '-';
}
const now = dayjs();
const diffSeconds = now.diff(submissionDate, 'seconds');
+ if (diffSeconds < 0) {
+ return '0s';
+ }
+
if (diffSeconds < 60) {
return `${diffSeconds}s`;🤖 Prompt for AI Agents
In
`@src/main/webapp/app/buildagent/build-queue/tables/queued-jobs-table/queued-jobs-table.component.ts`
around lines 87 - 105, The calculateWaitingTime function can produce negative
outputs when diffSeconds is negative due to client/server clock skew; modify
calculateWaitingTime to detect when diffSeconds < 0 (use the existing
diffSeconds variable) and treat it as zero (e.g., return "0s" or clamp
diffSeconds to 0) before the existing formatting logic so the function never
returns negative durations.
- Update build-overview.component.spec.ts to expect 1 decimal place format ('0.0s') instead of 3 ('0.000s')
- Update build-job-detail.component.spec.ts to expect formatted string for queueWaitTime ('5.0s') instead of number
Co-Authored-By: Claude Opus 4.5 <[email protected]>
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||||||||
|
@krusche Your PR description needs attention before it can be reviewed: Issues Found
How to Fix
This check validates that your PR description follows the PR template. A complete description helps reviewers understand your changes and speeds up the review process.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (1)
485-565: Remove bracket requirement in address format check to support both IPv4 and IPv6 formats.The
isClusterMemberAddressmethod requires addresses to contain]:—a format that only matches IPv6 (e.g.,[::1]:5701). However,getClusterMemberAddresses()uses Hazelcast'sAddress.toString(), which returns IPv4 addresses without brackets (e.g.,127.0.0.1:5701). This causes offline IPv4 cluster members to bypass cleanup since they fail the format check.Update the condition to accept both formats:
Proposed fix
- if (address == null || !address.contains("]:")) { + if (address == null) { return false; } - // Extract port from the address (format: [host]:port) + // Extract port from the address (format: [host]:port or host:port)- * Extracts the port from an address in [host]:port format. + * Extracts the port from an address in [host]:port or host:port format.
🤖 Fix all issues with AI agents
In
`@src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java`:
- Around line 233-281: tryInitialize() can be invoked concurrently causing
duplicate listeners; change the gating to use an atomic CAS around
initialization (e.g., use initialized.compareAndSet(false, true) or introduce a
separate AtomicBoolean initializing) so only one thread proceeds to register the
topic listener. Concretely: at the start of tryInitialize() attempt to
atomically claim initialization (if compareAndSet returns false, return
initialized.get() result immediately), perform the getCanceledBuildJobsTopic()
and listener registration while holding the claim, set cancelListenerId as now,
and only on success leave initialized true; on any exception or failure clear
the claim (reset the atomic flag or set initialized back to false) and return
false. Ensure existing cancelListenerId removal logic and
jobLifecycleLock/usages (runningFutures, cancelBuildJob) remain unchanged and
are executed only by the successful initializer.
In
`@src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java`:
- Around line 243-312: The tryInitialize() method can run concurrently causing
duplicate listeners and scheduled tasks; fix it by making the method execution
single-threaded: at the top use an atomic CAS (initialized.compareAndSet(false,
true)) or a dedicated ReentrantLock to gate execution so only one initializer
proceeds, and if CAS fails return true/false appropriately; ensure you only set
initialized after full success (i.e., use a temporary flag or set back to false
on failure), and before scheduling a new task check/cancel existing
scheduledFuture and clear listenerId/pauseListenerId/resumeListenerId only
inside the guarded section; update references in tryInitialize(), listenerId,
pauseListenerId, resumeListenerId, scheduledFuture,
taskScheduler.scheduleAtFixedRate and distributedDataAccessService usages
accordingly.
🧹 Nitpick comments (2)
src/main/webapp/app/buildagent/build-queue/build-overview.component.ts (2)
275-302: Consider handling duplicate job updates more defensively.The
handleFinishedBuildJobUpdatemethod filters out duplicate jobs (line 294), but if the same job arrives multiple times in quick succession before the filter runs, there's a brief window for duplicates. This is a minor edge case since the filter handles it, but consider adding a check at the start:const currentJobs = this.finishedBuildJobs(); if (currentJobs.some(job => job.id === finishedBuildJob.id)) { return; // Already have this job }Also, the method doesn't verify that the new job matches the current sort order assumption (descending by
buildSubmissionDate). If the user changes sorting, prepending may place the job incorrectly.
344-348: Add error handling toloadBuildAgents().Unlike other data-loading methods in this component (e.g.,
loadFinishedBuildJobs),loadBuildAgents()lacks error handling. If the API call fails, the error is silently swallowed. Consider adding an error handler for consistency:🛠️ Suggested fix
loadBuildAgents() { - this.buildAgentsService.getBuildAgentSummary().subscribe((agents) => { - this.buildAgents.set(agents); + this.buildAgentsService.getBuildAgentSummary().subscribe({ + next: (agents) => { + this.buildAgents.set(agents); + }, + error: (error: HttpErrorResponse) => { + onError(this.alertService, error); + }, }); }
| /** | ||
| * Attempts to initialize the cancel listener for build jobs. | ||
| * <p> | ||
| * This method checks if the Hazelcast client is connected to the cluster before | ||
| * attempting to access distributed data structures. If not connected, it returns | ||
| * false so the caller can retry later. | ||
| * | ||
| * @return true if initialization succeeded, false if not connected to cluster | ||
| */ | ||
| private boolean tryInitialize() { | ||
| if (initialized.get()) { | ||
| return true; | ||
| } | ||
|
|
||
| if (!distributedDataAccessService.isConnectedToCluster()) { | ||
| log.debug("Cannot initialize BuildJobManagementService: not connected to Hazelcast cluster yet"); | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| var canceledBuildJobsTopic = distributedDataAccessService.getCanceledBuildJobsTopic(); | ||
|
|
||
| // Remove old listener if it exists (prevents duplicate listeners on reconnection) | ||
| if (cancelListenerId != null) { | ||
| canceledBuildJobsTopic.removeMessageListener(cancelListenerId); | ||
| cancelListenerId = null; | ||
| } | ||
|
|
||
| cancelListenerId = canceledBuildJobsTopic.addMessageListener(buildJobId -> { | ||
| jobLifecycleLock.lock(); | ||
| try { | ||
| if (runningFutures.containsKey(buildJobId)) { | ||
| cancelBuildJob(buildJobId); | ||
| } | ||
| } | ||
| finally { | ||
| jobLifecycleLock.unlock(); | ||
| } | ||
| }); | ||
|
|
||
| initialized.set(true); | ||
| log.info("BuildJobManagementService initialized successfully - cancel listener registered"); | ||
| return true; | ||
| } | ||
| catch (Exception e) { | ||
| // This can happen if the connection is lost between the check and the access | ||
| log.warn("Failed to initialize BuildJobManagementService: {}. Will retry.", e.getMessage()); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Prevent concurrent tryInitialize() executions.
The retry task and the connection listener can call tryInitialize() concurrently; both can pass initialized.get() and register duplicate topic listeners (Line 242-271). Synchronize the method or gate with a CAS.
🔧 Proposed fix
- private boolean tryInitialize() {
+ private synchronized boolean tryInitialize() {🤖 Prompt for AI Agents
In
`@src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java`
around lines 233 - 281, tryInitialize() can be invoked concurrently causing
duplicate listeners; change the gating to use an atomic CAS around
initialization (e.g., use initialized.compareAndSet(false, true) or introduce a
separate AtomicBoolean initializing) so only one thread proceeds to register the
topic listener. Concretely: at the start of tryInitialize() attempt to
atomically claim initialization (if compareAndSet returns false, return
initialized.get() result immediately), perform the getCanceledBuildJobsTopic()
and listener registration while holding the claim, set cancelListenerId as now,
and only on success leave initialized true; on any exception or failure clear
the claim (reset the atomic flag or set initialized back to false) and return
false. Ensure existing cancelListenerId removal logic and
jobLifecycleLock/usages (runningFutures, cancelBuildJob) remain unchanged and
are executed only by the successful initializer.
| /** | ||
| * Attempts to initialize the distributed listeners and scheduled tasks. | ||
| * <p> | ||
| * This method checks if the Hazelcast client is connected to the cluster before | ||
| * attempting to access distributed data structures. If not connected, it returns | ||
| * false so the caller can retry later. | ||
| * | ||
| * @return true if initialization succeeded, false if not connected to cluster | ||
| */ | ||
| private boolean tryInitialize() { | ||
| if (initialized.get()) { | ||
| return true; | ||
| } | ||
|
|
||
| if (!distributedDataAccessService.isConnectedToCluster()) { | ||
| log.debug("Cannot initialize SharedQueueProcessingService: not connected to Hazelcast cluster yet"); | ||
| return false; | ||
| } | ||
|
|
||
| distributedDataAccessService.getPauseBuildAgentTopic().addMessageListener(buildAgentName -> { | ||
| if (buildAgentShortName.equals(buildAgentName)) { | ||
| pauseBuildAgent(false); | ||
| try { | ||
| // Remove listener if already present (for idempotency) | ||
| if (this.listenerId != null) { | ||
| distributedDataAccessService.getDistributedBuildJobQueue().removeListener(this.listenerId); | ||
| } | ||
| }); | ||
|
|
||
| distributedDataAccessService.getResumeBuildAgentTopic().addMessageListener(buildAgentName -> { | ||
| if (buildAgentShortName.equals(buildAgentName)) { | ||
| resumeBuildAgent(); | ||
| log.info("Adding item listener to Hazelcast distributed build job queue for build agent with address {}", distributedDataAccessService.getLocalMemberAddress()); | ||
| this.listenerId = this.distributedDataAccessService.getDistributedBuildJobQueue().addItemListener(new QueuedBuildJobItemListener()); | ||
|
|
||
| /* | ||
| * Check every 5 seconds whether the node has at least one thread available for a new build job. | ||
| * If so, process the next build job. | ||
| * This is a backup mechanism in case the build queue is not empty, no new build jobs are entering the queue and the | ||
| * node otherwise stopped checking for build jobs in the queue. | ||
| */ | ||
| scheduledFuture = taskScheduler.scheduleAtFixedRate(this::checkAvailabilityAndProcessNextBuild, BUILD_CHECK_AVAILABILITY_INTERVAL); | ||
|
|
||
| var pauseTopic = distributedDataAccessService.getPauseBuildAgentTopic(); | ||
| var resumeTopic = distributedDataAccessService.getResumeBuildAgentTopic(); | ||
|
|
||
| // Remove old listeners if they exist (prevents duplicate listeners on reconnection) | ||
| if (pauseListenerId != null) { | ||
| pauseTopic.removeMessageListener(pauseListenerId); | ||
| pauseListenerId = null; | ||
| } | ||
| }); | ||
| if (resumeListenerId != null) { | ||
| resumeTopic.removeMessageListener(resumeListenerId); | ||
| resumeListenerId = null; | ||
| } | ||
|
|
||
| pauseListenerId = pauseTopic.addMessageListener(buildAgentName -> { | ||
| if (buildAgentShortName.equals(buildAgentName)) { | ||
| pauseBuildAgent(false); | ||
| } | ||
| }); | ||
|
|
||
| resumeListenerId = resumeTopic.addMessageListener(buildAgentName -> { | ||
| if (buildAgentShortName.equals(buildAgentName)) { | ||
| resumeBuildAgent(); | ||
| } | ||
| }); | ||
|
|
||
| initialized.set(true); | ||
| log.info("SharedQueueProcessingService initialized successfully - listening for build jobs"); | ||
| return true; | ||
| } | ||
| catch (Exception e) { | ||
| // This can happen if the connection is lost between the check and the access | ||
| log.warn("Failed to initialize SharedQueueProcessingService: {}. Will retry.", e.getMessage()); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Guard tryInitialize() against concurrent execution.
Retry scheduling and connection callbacks can race, leading to duplicate queue/topic listeners and multiple scheduled availability checks (Line 252-305). Synchronize the method or gate with a CAS.
🔧 Proposed fix
- private boolean tryInitialize() {
+ private synchronized boolean tryInitialize() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Attempts to initialize the distributed listeners and scheduled tasks. | |
| * <p> | |
| * This method checks if the Hazelcast client is connected to the cluster before | |
| * attempting to access distributed data structures. If not connected, it returns | |
| * false so the caller can retry later. | |
| * | |
| * @return true if initialization succeeded, false if not connected to cluster | |
| */ | |
| private boolean tryInitialize() { | |
| if (initialized.get()) { | |
| return true; | |
| } | |
| if (!distributedDataAccessService.isConnectedToCluster()) { | |
| log.debug("Cannot initialize SharedQueueProcessingService: not connected to Hazelcast cluster yet"); | |
| return false; | |
| } | |
| distributedDataAccessService.getPauseBuildAgentTopic().addMessageListener(buildAgentName -> { | |
| if (buildAgentShortName.equals(buildAgentName)) { | |
| pauseBuildAgent(false); | |
| try { | |
| // Remove listener if already present (for idempotency) | |
| if (this.listenerId != null) { | |
| distributedDataAccessService.getDistributedBuildJobQueue().removeListener(this.listenerId); | |
| } | |
| }); | |
| distributedDataAccessService.getResumeBuildAgentTopic().addMessageListener(buildAgentName -> { | |
| if (buildAgentShortName.equals(buildAgentName)) { | |
| resumeBuildAgent(); | |
| log.info("Adding item listener to Hazelcast distributed build job queue for build agent with address {}", distributedDataAccessService.getLocalMemberAddress()); | |
| this.listenerId = this.distributedDataAccessService.getDistributedBuildJobQueue().addItemListener(new QueuedBuildJobItemListener()); | |
| /* | |
| * Check every 5 seconds whether the node has at least one thread available for a new build job. | |
| * If so, process the next build job. | |
| * This is a backup mechanism in case the build queue is not empty, no new build jobs are entering the queue and the | |
| * node otherwise stopped checking for build jobs in the queue. | |
| */ | |
| scheduledFuture = taskScheduler.scheduleAtFixedRate(this::checkAvailabilityAndProcessNextBuild, BUILD_CHECK_AVAILABILITY_INTERVAL); | |
| var pauseTopic = distributedDataAccessService.getPauseBuildAgentTopic(); | |
| var resumeTopic = distributedDataAccessService.getResumeBuildAgentTopic(); | |
| // Remove old listeners if they exist (prevents duplicate listeners on reconnection) | |
| if (pauseListenerId != null) { | |
| pauseTopic.removeMessageListener(pauseListenerId); | |
| pauseListenerId = null; | |
| } | |
| }); | |
| if (resumeListenerId != null) { | |
| resumeTopic.removeMessageListener(resumeListenerId); | |
| resumeListenerId = null; | |
| } | |
| pauseListenerId = pauseTopic.addMessageListener(buildAgentName -> { | |
| if (buildAgentShortName.equals(buildAgentName)) { | |
| pauseBuildAgent(false); | |
| } | |
| }); | |
| resumeListenerId = resumeTopic.addMessageListener(buildAgentName -> { | |
| if (buildAgentShortName.equals(buildAgentName)) { | |
| resumeBuildAgent(); | |
| } | |
| }); | |
| initialized.set(true); | |
| log.info("SharedQueueProcessingService initialized successfully - listening for build jobs"); | |
| return true; | |
| } | |
| catch (Exception e) { | |
| // This can happen if the connection is lost between the check and the access | |
| log.warn("Failed to initialize SharedQueueProcessingService: {}. Will retry.", e.getMessage()); | |
| return false; | |
| } | |
| /** | |
| * Attempts to initialize the distributed listeners and scheduled tasks. | |
| * <p> | |
| * This method checks if the Hazelcast client is connected to the cluster before | |
| * attempting to access distributed data structures. If not connected, it returns | |
| * false so the caller can retry later. | |
| * | |
| * `@return` true if initialization succeeded, false if not connected to cluster | |
| */ | |
| private synchronized boolean tryInitialize() { | |
| if (initialized.get()) { | |
| return true; | |
| } | |
| if (!distributedDataAccessService.isConnectedToCluster()) { | |
| log.debug("Cannot initialize SharedQueueProcessingService: not connected to Hazelcast cluster yet"); | |
| return false; | |
| } | |
| try { | |
| // Remove listener if already present (for idempotency) | |
| if (this.listenerId != null) { | |
| distributedDataAccessService.getDistributedBuildJobQueue().removeListener(this.listenerId); | |
| } | |
| log.info("Adding item listener to Hazelcast distributed build job queue for build agent with address {}", distributedDataAccessService.getLocalMemberAddress()); | |
| this.listenerId = this.distributedDataAccessService.getDistributedBuildJobQueue().addItemListener(new QueuedBuildJobItemListener()); | |
| /* | |
| * Check every 5 seconds whether the node has at least one thread available for a new build job. | |
| * If so, process the next build job. | |
| * This is a backup mechanism in case the build queue is not empty, no new build jobs are entering the queue and the | |
| * node otherwise stopped checking for build jobs in the queue. | |
| */ | |
| scheduledFuture = taskScheduler.scheduleAtFixedRate(this::checkAvailabilityAndProcessNextBuild, BUILD_CHECK_AVAILABILITY_INTERVAL); | |
| var pauseTopic = distributedDataAccessService.getPauseBuildAgentTopic(); | |
| var resumeTopic = distributedDataAccessService.getResumeBuildAgentTopic(); | |
| // Remove old listeners if they exist (prevents duplicate listeners on reconnection) | |
| if (pauseListenerId != null) { | |
| pauseTopic.removeMessageListener(pauseListenerId); | |
| pauseListenerId = null; | |
| } | |
| if (resumeListenerId != null) { | |
| resumeTopic.removeMessageListener(resumeListenerId); | |
| resumeListenerId = null; | |
| } | |
| pauseListenerId = pauseTopic.addMessageListener(buildAgentName -> { | |
| if (buildAgentShortName.equals(buildAgentName)) { | |
| pauseBuildAgent(false); | |
| } | |
| }); | |
| resumeListenerId = resumeTopic.addMessageListener(buildAgentName -> { | |
| if (buildAgentShortName.equals(buildAgentName)) { | |
| resumeBuildAgent(); | |
| } | |
| }); | |
| initialized.set(true); | |
| log.info("SharedQueueProcessingService initialized successfully - listening for build jobs"); | |
| return true; | |
| } | |
| catch (Exception e) { | |
| // This can happen if the connection is lost between the check and the access | |
| log.warn("Failed to initialize SharedQueueProcessingService: {}. Will retry.", e.getMessage()); | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In
`@src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java`
around lines 243 - 312, The tryInitialize() method can run concurrently causing
duplicate listeners and scheduled tasks; fix it by making the method execution
single-threaded: at the top use an atomic CAS (initialized.compareAndSet(false,
true)) or a dedicated ReentrantLock to gate execution so only one initializer
proceeds, and if CAS fails return true/false appropriately; ensure you only set
initialized after full success (i.e., use a temporary flag or set back to false
on failure), and before scheduling a new task check/cancel existing
scheduledFuture and clear listenerId/pauseListenerId/resumeListenerId only
inside the guarded section; update references in tryInitialize(), listenerId,
pauseListenerId, resumeListenerId, scheduledFuture,
taskScheduler.scheduleAtFixedRate and distributedDataAccessService usages
accordingly.
Summary
Improves Hazelcast cluster stability by:
Improves usability and user experience:
Checklist
General
Server
Changes affecting Programming Exercises
Motivation and Context
During large scale exams with thousand students, build agents could become unresponsive, which could cause cascading cluster failures due to the default 60-second heartbeat timeout. The architecture with build agents as lite cluster members meant that an unresponsive build agent could potentially affect the entire cluster's stability.
This PR addresses these issues by:
Additionally, the build queue monitoring UI has been improved:
Description
Build Agent Client Mode
Cluster Stability Configuration
Hazelcast 5.6.0 Upgrade
HazelcastObjectExtractionConfigurationto work around a known issueBuild Job Detail Page (New)
GET /api/admin/build-job/{buildJobId}andGET /api/courses/{courseId}/build-job/{buildJobId}to retrieve individual build jobs/topic/admin/build-job/{jobId}and/topic/courses/{courseId}/build-job/{jobId}for real-time updatesSimplified Finished Builds Table
Previous Configuration (Lite Members)
Problems:
• e.g. 24 members = 24×23 = 552 heartbeat connections, in general
(N+M)×(N+M-1)• One unresponsive build agent could trigger "Suspecting Member" for ALL nodes
• Cascading failures: cluster instability could affect core nodes
• 60-second default timeout = long detection time
New Configuration (Client Mode)
• Auto-discovery via Service Registry (Eureka)
• Automatic reconnection with exponential backoff
• No heartbeat overhead between build agents
Benefits:
• Core cluster: e.g. 4 members = 4×3 = 12 heartbeat connections (vs 552), in general
N×(N-1)• Build agent failure ≠ cluster event (just a disconnected client)
• Core cluster stability unaffected by build agent issues
• Phi-accrual failure detector + 15s timeout for faster detection
Data Flow (Unchanged)
All IQueue, ITopic, IMap operations work identically for clients!
Steps for Testing
Prerequisites:
Hazelcast Cluster Testing
Single-node Testing
coreandbuildagentprofilesBuild Job Detail Page Testing
/admin/build-queue/job-details?jobId=<id>/course-management/<courseId>/build-queue/job-details?jobId=<id>Review Progress
good performance even for very large courses with more than 2000 students.
Code Review
Manual Tests
Summary by CodeRabbit
Dependencies
Bug Fixes
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.