Skip to content

Fix SciTag WAN TPC marker generation and validation #8044

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kofemann merged 4 commits into from
Mar 20, 2026
Merged

Fix SciTag WAN TPC marker generation and validation #8044

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cfa8482
Fix SciTag WAN TPC marker generation and validation
ShawnMcKee Mar 17, 2026
5cef4f7
xrootd: fix logSciTagsRequest called before cell address initialization
ShawnMcKee Mar 17, 2026
db22839
firefly: address PR 8044 review fixes
ShawnMcKee Mar 18, 2026
d0be764
firefly: address latest PR 8044 inline feedback
ShawnMcKee Mar 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 55 additions & 27 deletions modules/dcache-webdav/src/main/java/org/dcache/webdav/DcacheResourceFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,11 +169,28 @@
* This ResourceFactory exposes the dCache name space through the Milton WebDAV framework.
*/
public class DcacheResourceFactory
extends AbstractCellComponent
implements ResourceFactory, CellMessageReceiver, CellCommandListener, CellInfoProvider {
extends AbstractCellComponent
implements ResourceFactory, CellMessageReceiver, CellCommandListener, CellInfoProvider {

private static final Logger LOGGER =
LoggerFactory.getLogger(DcacheResourceFactory.class);
LoggerFactory.getLogger(DcacheResourceFactory.class);
private static final Logger SCITAGS_LOGGER =
Copy link
Member

@kofemann kofemann Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure that we need extra logger for sitags

Copy link
Contributor Author

@ShawnMcKee ShawnMcKee Mar 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for raising this. After operational debugging and validation, I would like to keep a dedicated org.dcache.scitags logger. It was important for diagnosing and fixing the firefly marker issue. This remains debug-gated, so it stays quiet by default, and can be enabled for targeted troubleshooting when needed.

LoggerFactory.getLogger("org.dcache.scitags");

static Optional<String> findHeaderIgnoreCase(HttpServletRequest request,
String expectedHeaderName) {
Enumeration<String> headerNames = request.getHeaderNames();
if (headerNames != null) {
while (headerNames.hasMoreElements()) {
String actualHeaderName = headerNames.nextElement();
if (actualHeaderName.equalsIgnoreCase(expectedHeaderName)) {
return Optional.ofNullable(request.getHeader(actualHeaderName));
}
}
}

return Optional.ofNullable(request.getHeader(expectedHeaderName));
}

private static final XMLOutputFactory XML_OUTPUT_FACTORY = XMLOutputFactory.newFactory();

Expand Down Expand Up @@ -1697,11 +1714,11 @@ private class HttpTransfer extends RedirectedTransfer<String> {
/**
* The original request path that will be passed to pool for fall-back redirect.
*/
private final String _requestPath;
private String _transferTag = "";
private final String _requestPath;
private String _transferTag = "";

private static final String HEADER_SCITAG = "SciTag";
private static final String HEADER_TRANSFER_HEADER_SCITAG = "TransferHeaderSciTag";
private static final String HEADER_SCITAG = "SciTag";
private static final String HEADER_TRANSFER_HEADER_SCITAG = "TransferHeaderSciTag";
private static final String[] SCITAG_HEADERS = {
HEADER_SCITAG,
HEADER_TRANSFER_HEADER_SCITAG
Expand All @@ -1720,36 +1737,47 @@ public HttpTransfer(PnfsHandler pnfs, Subject subject,
}

private String readTransferTag(HttpServletRequest request) {
String door = getCellName() + '@' + getCellDomainName();

// SciTag takes precedence because it is checked first.
for (String header : SCITAG_HEADERS) {
String transferTag = request.getHeader(header);
if (transferTag != null && !transferTag.isBlank()) {
String trimmed = transferTag.trim();
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("{} header found: {} (from client={})",
header, trimmed, request.getRemoteAddr());
}
return trimmed;
var transferTag = findHeaderIgnoreCase(request, header)
.map(String::trim)
.filter(tag -> !tag.isEmpty());
if (transferTag.isPresent()) {
logSciTagsRequest(request, door, header + "-header", transferTag.get());
return transferTag.get();
}
}

String flowFromQuery = request.getParameter("scitag.flow");
if (flowFromQuery != null && !flowFromQuery.isBlank()) {
String trimmed = flowFromQuery.trim();
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("scitag.flow query parameter found: {} (from client={})",
trimmed, request.getRemoteAddr());
}
return trimmed;
var flowFromQuery = Optional.ofNullable(request.getParameter("scitag.flow"))
.map(String::trim)
.filter(tag -> !tag.isEmpty());
if (flowFromQuery.isPresent()) {
logSciTagsRequest(request, door, "scitag.flow-query", flowFromQuery.get());
return flowFromQuery.get();
}

if (LOGGER.isDebugEnabled()) {
LOGGER.debug("No SciTag header/parameter found in request (client={})",
request.getRemoteAddr());
}
logSciTagsRequest(request, door, "none", "");
return "";
}

private static void logSciTagsRequest(HttpServletRequest request, String door,
String tagSource, String transferTag) {
if (SCITAGS_LOGGER.isDebugEnabled()) {
SCITAGS_LOGGER.debug(
"scitags event=request protocol={} door={} remote={} method={} alias={} local={} tagSource={} transferTag={}",
request.isSecure() ? PROTOCOL_INFO_SSL_NAME : PROTOCOL_INFO_NAME,
door,
request.getRemoteAddr(),
request.getMethod(),
request.getServerName(),
request.getLocalAddr(),
tagSource,
transferTag.isEmpty() ? "-" : transferTag);
}
}

protected ProtocolInfo createProtocolInfo(InetSocketAddress address) {
List<ChecksumType> wantedChecksums = _wantedChecksum == null
? Collections.emptyList()
Expand Down
55 changes: 55 additions & 0 deletions modules/dcache-webdav/src/test/java/org/dcache/webdav/DcacheResourceFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package org.dcache.webdav;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.mock;

import java.util.Collections;
import java.util.List;
import java.util.Optional;
import javax.servlet.http.HttpServletRequest;
import org.junit.Test;

public class DcacheResourceFactoryTest {

@Test
public void shouldFindSciTagHeaderIgnoringCase() {
HttpServletRequest request = mock(HttpServletRequest.class);
given(request.getHeaderNames()).willReturn(Collections.enumeration(List.of("scitag")));
given(request.getHeader("scitag")).willReturn("313");

Optional<String> header =
DcacheResourceFactory.findHeaderIgnoreCase(request, "SciTag");

assertThat(header.isPresent(), is(true));
assertThat(header.get(), is("313"));
}

@Test
public void shouldFindTransferHeaderSciTagIgnoringCase() {
HttpServletRequest request = mock(HttpServletRequest.class);
given(request.getHeaderNames()).willReturn(
Collections.enumeration(List.of("transferheaderscitag")));
given(request.getHeader("transferheaderscitag")).willReturn("777");

Optional<String> header =
DcacheResourceFactory.findHeaderIgnoreCase(request, "TransferHeaderSciTag");

assertThat(header.isPresent(), is(true));
assertThat(header.get(), is("777"));
}

@Test
public void shouldFallbackToServletHeaderLookup() {
HttpServletRequest request = mock(HttpServletRequest.class);
given(request.getHeaderNames()).willReturn(null);
given(request.getHeader("SciTag")).willReturn("313");

Optional<String> header =
DcacheResourceFactory.findHeaderIgnoreCase(request, "SciTag");

assertThat(header.isPresent(), is(true));
assertThat(header.get(), is("313"));
}
}
1 change: 1 addition & 0 deletions modules/dcache-xrootd/src/main/java/org/dcache/xrootd/door/XrootdDoor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,7 @@ public synchronized void finished(CacheException error) {
transfer.setKafkaSender(_kafkaSender);
transfer.setTriedHosts(tried);
transfer.setProxiedTransfer(proxied);
transfer.logSciTagsRequest(opaque);
return transfer;
}

Expand Down
36 changes: 27 additions & 9 deletions modules/dcache-xrootd/src/main/java/org/dcache/xrootd/door/XrootdTransfer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.dcache.xrootd.door;

import static com.google.common.net.InetAddresses.toUriString;
import static java.util.Objects.requireNonNull;

import diskCacheV111.util.FsPath;
Expand All @@ -26,6 +27,7 @@
public class XrootdTransfer extends RedirectedTransfer<InetSocketAddress> {

private static final Logger LOGGER = LoggerFactory.getLogger(XrootdTransfer.class);
private static final Logger SCITAGS_LOGGER = LoggerFactory.getLogger("org.dcache.scitags");

private UUID _uuid;
private InetSocketAddress _doorAddress;
Expand All @@ -43,15 +45,6 @@ public XrootdTransfer(PnfsHandler pnfs, Subject subject,
this.restriction = requireNonNull(restriction);
tpcInfo = new XrootdTpcInfo(opaque);
_transferTag = opaque.getOrDefault("scitag.flow", "");
if (LOGGER.isDebugEnabled()) {
if (!_transferTag.isEmpty()) {
LOGGER.debug("scitag.flow parameter found: {}", _transferTag);
} else if (opaque.containsKey("scitag.flow")) {
LOGGER.debug("scitag.flow parameter found but empty");
} else {
LOGGER.debug("No scitag.flow parameter in this request");
}
}
try {
tpcInfo.setUid(Subjects.getUid(subject));
} catch (NoSuchElementException e) {
Expand Down Expand Up @@ -96,6 +89,31 @@ protected synchronized ProtocolInfo createProtocolInfo() {
return createXrootdProtocolInfo();
}

void logSciTagsRequest(Map<String, String> opaque) {
if (SCITAGS_LOGGER.isDebugEnabled()) {
String tagSource = !_transferTag.isEmpty()
? "scitag.flow"
: opaque.containsKey("scitag.flow") ? "scitag.flow-empty" : "none";
SCITAGS_LOGGER.debug(
"scitags event=request protocol=xrootd door={} remote={} tagSource={} transferTag={}",
getCellName() + '@' + getDomainName(),
formatAddress(getClientAddress()),
tagSource,
_transferTag.isEmpty() ? "-" : _transferTag);
}
}

private String formatAddress(InetSocketAddress address) {
if (address == null) {
return "-";
}

var inetAddress = address.getAddress();
return inetAddress == null
? address.getHostString()
: toUriString(inetAddress);
}

@Override
protected ProtocolInfo getProtocolInfoForPoolManager() {
ProtocolInfo info = createProtocolInfo();
Expand Down
35 changes: 35 additions & 0 deletions ...es/dcache/src/main/java/org/dcache/pool/classic/AbstractMoverProtocolTransferService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.dcache.pool.movers.Mover;
import org.dcache.pool.movers.MoverProtocol;
import org.dcache.pool.movers.MoverProtocolMover;
import org.dcache.pool.movers.TransferLifeCycle;
import org.dcache.pool.repository.ReplicaDescriptor;
import org.dcache.pool.repository.RepositoryChannel;
import org.dcache.util.CDCExecutorServiceDecorator;
Expand All @@ -52,19 +53,26 @@ public abstract class AbstractMoverProtocolTransferService

private static final Logger LOGGER =
LoggerFactory.getLogger(MoverMapTransferService.class);
private static final Logger SCITAGS_LOGGER =
LoggerFactory.getLogger("org.dcache.scitags");
private final ExecutorService _executor =
new CDCExecutorServiceDecorator<>(
Executors.newCachedThreadPool(
new ThreadFactoryBuilder().setNameFormat(
getClass().getSimpleName() + "-transfer-service-%d").build()));
private PostTransferService _postTransferService;
private TransferLifeCycle _transferLifeCycle;


@Required
public void setPostTransferService(PostTransferService postTransferService) {
_postTransferService = postTransferService;
}

public void setTransferLifeCycle(TransferLifeCycle transferLifeCycle) {
_transferLifeCycle = transferLifeCycle;
}

@Override
public Mover<?> createMover(ReplicaDescriptor handle, PoolIoFileMessage message,
CellPath pathToDoor)
Expand Down Expand Up @@ -155,10 +163,22 @@ public void run() {
_completionHandler.completed(null, null);

} catch (InterruptedException e) {
SCITAGS_LOGGER.debug(
"scitags lifecycle=start abort reason=interrupted protocol={} pnfsid={} transferTag={} message={}",
protocolName(),
_mover.getFileAttributes().getPnfsId(),
transferTag(),
formatError(e));
InterruptedException why = _explanation == null ? e :
(InterruptedException) (new InterruptedException(_explanation).initCause(e));
_completionHandler.failed(why, null);
} catch (Throwable t) {
SCITAGS_LOGGER.debug(
"scitags lifecycle=start abort reason=execution-failed protocol={} pnfsid={} transferTag={} message={}",
protocolName(),
_mover.getFileAttributes().getPnfsId(),
transferTag(),
formatError(t));
_completionHandler.failed(t, null);
}
}
Expand Down Expand Up @@ -200,6 +220,21 @@ private void runMoverForWrite(RepositoryChannel fileIoChannel) throws Exception
}
}

private String protocolName() {
return _mover.getProtocolInfo().getProtocol().toLowerCase();
}

private String transferTag() {
String transferTag = _mover.getProtocolInfo().getTransferTag();
return transferTag == null || transferTag.isEmpty() ? "-" : transferTag;
}

private String formatError(Throwable t) {
Copy link
Member

@kofemann kofemann Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use our utility method org.dcache.util.Exceptions#messageOrClassName

return t instanceof Exception
? Exceptions.messageOrClassName((Exception) t)
: t.getClass().getName();
}

private synchronized void setThread() throws InterruptedException {
if (_needInterruption) {
throw new InterruptedException("Thread interrupted before execution");
Expand Down
Loading
Loading