Allow multiple LogStorage with primary and secondaries

[jgreffe]

See jenkinsci/pipeline-cloudwatch-logs-plugin#132 and jenkinsci/workflow-durable-task-step-plugin#464 for downstream changes. This PR is a follow-up to #406 (comment).

This PR adjusts the LogStorageFactory API for two main purposes:

• To allow users to explicitly select a particular LogStorageFactory in case they have multiple plugins installed that implement LogStorageFactory
• To allow the implementation of a new TeeLogStorageFactory, which allows users to configure a "primary" LogStorageFactory that handles reads and writes, and a "secondary" factory that only handle copies of all writes
  • This prevents plugins from having to reimplement their own Tee logic internally (as done by the existing opentelemetry plugin)
  • It also allows the implementation of write-only LogStorage implementations that simply want to copy log content elsewhere without reading it back into Jenkins, as considered in Security-2401 Fix credentials leakage to Splunk splunk-devops-plugin#17

Specifically, this PR converts LogStorageFactory from an ExtensionPoint to a Describable, and introduces LogStorageFactoryDescriptor. It also introduces a GlobalConfiguration for Pipeline logging and a new TeeLogStorageFactory logger implementation.

Monosnap.screencast.2025-07-18.15-01-23.mp4

This change is not backwards compatible. All plugins that implement LogStorageFactory will need to be updated and released shortly after this PR is released to avoid compatibility issues.

Testing done

See new and existing automated tests here and in downstream PRs. This PR has also been tested manually.

Submitter checklist

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests that demonstrate the feature works or the issue is fixed

[dwnusbaum]

Just to note: While I was looking at the code yesterday I realized the logic here is a bit confusing. We close the delegate TaskListeners, but we don't actually close the TeeOutputStream or the PrintStream returned by getLogger(). At first I thought maybe we could simplify by just calling getLogger().close(), which would eventually close the TeeOutputStream and we could get rid of the rest of the method, but at least BufferedBuildListener and CloudWatchSender have special close methods that do more than just closing the internal streams, so we really do need to try to close the TaskListeners themselves.

This means that after a call to close(), getLogger() and outputStream will still exist as unclosed streams even after their delegates have been been closed, which is a bit weird. We might be able to call getLogger().close() and then close the listeners, but I can't remember if any of the relevant streams have problems with being closed twice. I would maybe look into this a bit just to make sure whether the TeeOutputStream not being closed is ok.

[jgreffe]

If I update with something like:

        Exception exception = null;
        if (outputStream != null) {
            try {
                outputStream.close();
            } catch (IOException e) {
                exception = e;
            }
        }

then TeeLogStorageTest#smokes fails with:

org.opentest4j.MultipleFailuresError: Multiple Failures (2 failures)
	org.junit.ComparisonFailure: expected:<[<a href='http://nowhere.net/'>nikde</a>
]> but was:<[]>
	java.lang.AssertionError: 
Expected: is "starting\none #1\ntwo #1\ntwo #2\ninterrupting\none #2\none #3\npausing\nresuming\none #4\nthree #1\nending\n"
     but: was "starting\none #1\ntwo #1\ntwo #2\ninterrupting\none #2\none #3\npausing\nresuming\none #4\nthree #1\nending\nha:////4AM+Xbq6l0DXt+Aa5ectJ4m2Ny0f1G1cNAXESjnHxoh7AAAAlB+LCAAAAAAAAP9b85aBtbiIQSajNKU4P08vOT+vOD8nVc+jsiC1KCczL9svvyTVzHb1RttJBUeZGJg8GdhyUvPSSzJ8GJhLi3JKGIR8shLLEvVzEvPS9YNLijLz0q0rihik0IxzhtAgwxgggJGJgaGiAMhgLWEQzigpKbDS18/LL89ILUrVy0st0QcAFd2f8JgAAAA=nikde\n"
	...
	Suppressed: org.junit.ComparisonFailure: expected:<[<a href='http://nowhere.net/'>nikde</a>
]> but was:<[]>
		at org.jenkinsci.plugins.workflow.log.LogStorageTestBase.assertLog(LogStorageTestBase.java:324)
		at org.jenkinsci.plugins.workflow.log.LogStorageTestBase.assertStepLog(LogStorageTestBase.java:305)
		at org.jenkinsci.plugins.workflow.log.LogStorageTestBase.smokes(LogStorageTestBase.java:155)

[dwnusbaum]

I think you specifically need to use getLogger().close(), not outputStream.close, maybe so that the PrintStream flushes its buffer and the stream, but I am not exactly sure why the former works but the latter does not without investigating more deeply. If things are ok without closing the PrintStream or TeeOutputStream itself that might be fine too, I would just check to make sure everything is getting GC'd as expected. Maybe there could be a difference in behavior if you tested writing lines without newlines or something like that, but I do not know.

[jgreffe]

Please see da22a49#diff-7edd00ed1b12a0d1e07adc240c962db00534506ab3043d8636f193ba84d7436cR51

[jgreffe]

But now with the additional getLogger().close(), the logger is flushed and closed, making TeeOutputStreamTest#primary_fails_close fail, and the close() method is called twice:

Breakpoint reached
	at org.jenkinsci.plugins.workflow.log.tee.TeeOutputStreamTest$4.close(TeeOutputStreamTest.java:118)
	at org.jenkinsci.plugins.workflow.log.tee.RemoteCustomFileLogStorage$Writer.close(RemoteCustomFileLogStorage.java:138)
	at org.jenkinsci.plugins.workflow.log.tee.TeeOutputStream$$Lambda/0x000000d80168a838.apply(Unknown Source:-1)
	at org.jenkinsci.plugins.workflow.log.tee.TeeOutputStream.handleAction(TeeOutputStream.java:45)
	at org.jenkinsci.plugins.workflow.log.tee.TeeOutputStream.close(TeeOutputStream.java:34)
	at java.io.PrintStream.implClose(PrintStream.java:500)
	at java.io.PrintStream.close(PrintStream.java:484)
	at org.jenkinsci.plugins.workflow.log.tee.TeeBuildListener.close(TeeBuildListener.java:51)
...
Breakpoint reached
	at org.jenkinsci.plugins.workflow.log.tee.TeeOutputStreamTest$4.close(TeeOutputStreamTest.java:118)
	at org.jenkinsci.plugins.workflow.log.tee.RemoteCustomFileLogStorage$Writer.close(RemoteCustomFileLogStorage.java:138)
	at org.jenkinsci.plugins.workflow.log.tee.RemoteCustomFileLogStorage$MyListener.close(RemoteCustomFileLogStorage.java:99)
	at org.jenkinsci.plugins.workflow.log.tee.TeeBuildListener.close(TeeBuildListener.java:55)
	at org.jenkinsci.plugins.workflow.log.tee.TeeOutputStreamTest.primary_fails_close(TeeOutputStreamTest.java:123)
	at java.lang.invoke.LambdaForm$DMH/0x000000d801218c00.invokeVirtual(LambdaForm$DMH:-1)
	at java.lang.invoke.LambdaForm$MH/0x000000d801324000.invoke(LambdaForm$MH:-1)
	at java.lang.invoke.Invokers$Holder.invokeExact_MT(Invokers$Holder:-1)

[dwnusbaum]

Yes, sorry, I was not saying that we definitely need it, only that if we do need it, we need to close the full PrintStream, not just the OutputStream, and yes it means the stream will be closed twice as I mentioned in #417 (comment). It's just not clear to me whether it is preferable to leave the outer streams open and just let them be cleaned up by GC after the delegate TaskListeners are closed, or to close the PrintStream, causing the TeeOutputStreamTest stream to be closed twice. I will try to investigate in more detail later today.

[dwnusbaum]

Ok, so now we close the stream twice, but everything seems to be ok IIUC. @jgreffe did you investigate to see if you prefer the behavior with the current code or how you had things prior to adding line 51?

[kellie-freeman]

I added some suggestions for the strings, but probably didn't catch all instances. Overall, I think the use of "Tee" and "factories" is a bit confusing, so I added some possible alternatives. I'll be OOTO until August 11th, so I added several options in most cases, depending on the route we may take.

[dwnusbaum]

Ok, so now we close the stream twice, but everything seems to be ok IIUC. @jgreffe did you investigate to see if you prefer the behavior with the current code or how you had things prior to adding line 51?

[kellie-freeman]

I added a few more minor string suggestions. If there are any that I missed, please let me know.

[kellie-freeman]

Suggested change

	This logger will be used to read and write Pipeline build logs.
	The primary logger is used to read and write Pipeline build logs.

[kellie-freeman]

Suggested change

	Pipeline build logs will be duplicated as they are written and sent to this logger. This logger will never be used to read logs.
	If a secondary logger is selected, Pipeline build logs are duplicated as they are written and sent to this logger. This logger is never used to read logs.

[kellie-freeman]

Suggested change

	description=Specify the multiple loggers order.
	description=Specify the primary and secondary Pipeline logger.

[kellie-freeman]

Suggested change

	return "A Log Storage Factory Mock";
	return "A mock Pipeline logger";

[kellie-freeman]

Suggested change

	return "Another Log Storage Factory Mock 2";
	return "Another mock Pipeline logger";