[beekld]

Makes FDv1 fallback TTL-based and never permanent, per DATASYSTEM v2 1.6 / CSFDV2 8.3 (sdk-specs#251).

Closes SDK-2986. Part of SDK-2935.

• The fallback TTL is read from the X-LD-FD-Fallback-TTL header and from goodbye.protocolFallbackTTL. A goodbye that carries protocolFallbackTTL is now a fallback directive in both streaming and polling. A goodbye without it is still only logged.
• A TTL in (0, 1h] is used as-is. A missing, malformed, zero, negative, or > 1h TTL means the 1h default minus uniform random [0, 30m].
• On fallback, the FDv2 synchronizers are paused, not removed. They are restored when the TTL expires, whether or not an FDv1 fallback synchronizer is configured. Without one, the status is now Interrupted (it used to be Off for good).
• API change: subsystems.DataInitializer.Fetch now returns a *time.Duration fallback TTL instead of a bool. DataSynchronizerResult.FallbackToFDv1 bool is replaced by FDv1FallbackTTL *time.Duration. In both, nil means no fallback was requested, and zero means no usable TTL was supplied.

Requirements

• I have added test coverage for new or changed functionality
• I have followed the repository's pull request submission guidelines
• I have validated my changes against all supported platform versions

Implementation details

Describe the solution you've provided

• internal/datasourcev2/helpers.go: fdv1FallbackFromHeaders and fdv1FallbackFromGoodbye parse the directive. Both return nil when there is no fallback. A TTL that can't be parsed gives a pointer to zero, which the data system treats as "use the default". subsystems.Goodbye.ProtocolFallbackTTL is a json.RawMessage, so a malformed value still counts as a fallback directive.
• Streaming: a goodbye with a TTL emits an Interrupted result with FDv1FallbackTTL set and closes the stream. The partial payload is discarded.
• Polling: PollingRequester.Request (internal) also returns the directive. A goodbye in a polling payload returns a goodbyeError with the directive.
• internal/datasystem: fdv1FallbackDuration applies the TTL rules. fallBackToFDv1 pauses the FDv2 slots (synchronizerList.fdv1Fallback) and starts a recovery timer. When it fires, the running FDv1 synchronizer is closed, and then fdv2Recovery unblocks the paused slots. Slots that failed permanently stay blocked. With no FDv1 synchronizer, the loop waits on the timer. Everything runs on the single synchronizer goroutine, and each synchronizer is closed before the next one starts, so FDv1 and FDv2 never write to the store at the same time.
• Same as the C++ reference (cpp-sdks#631): a valid server TTL is not jittered, and zero no longer means indefinite.

Deliberately unchanged

• If an initializer requests fallback and no synchronizer of any kind is configured, there is nothing to recover to, so the status still goes to Off (TestFDV2InitializerFallbackWithoutFDv1FallbackTransitionsToOff).
• Contract tests: the harness has an fdv1Fallback config but doesn't cover the TTL, so no contract-test changes.
• FDv1 synchronizers can't request fallback, so a fallback signal that arrives while already on FDv1 doesn't restart the TTL.

Describe alternatives you've considered

• Keeping Fetch returning bool and adding the TTL somewhere else: an initializer that falls back with an error returns no Basis, so the TTL would have nowhere to go. That's why the signature changed. DataSynchronizerResult could take an additive field, so it did.
• Using a bare TTL with zero meaning "no fallback": zero is a valid input that has to map to the default. A nil *time.Duration means "no fallback" instead, so there is no separate flag to keep in sync.

Testing

• Unit: header/goodbye parsing (TestFDv1FallbackFromHeaders, TestFDv1FallbackFromGoodbye). Valid TTLs come back unchanged and invalid ones land in [30m, 1h] (TestFDv1FallbackDuration*). Streaming/polling goodbye and header TTLs are covered. Recovery is tested with and without an FDv1 synchronizer, from both initializer and synchronizer fallback; the test checks the start/close order (internal/datasystem/fdv2_fdv1_recovery_test.go). Synchronizer list pause/restore is also covered.
• End-to-end: TestFDV2SynchronizerFallbackWithoutFDv1FallbackRecoversAfterTTL (replaces ...TransitionsToOff) uses a 1s header TTL and waits for the second request.
• make lint and go test ./... pass locally.

Link to Devin session: https://app.devin.ai/sessions/aeea2b28110a405c9b61c0ab2092e5a4
Open in Devin Desktop: https://app.devin.ai/desktop/session/aeea2b28110a405c9b61c0ab2092e5a4?variant=devin
Requested by: @beekld

[devin-ai-integration]

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

• Disable automatic comment, CI, and merge conflict monitoring

[beekld]

This seems not idiomatic. I mean, Fetch(ds DataSelector, ctx context.Context) (basis *Basis, fdv1Fallback FDv1Fallback, err error) uses *Basis as nil for an optional value, so I would expect it to use a *FDv1Fallback that is nil when fallback wasn't requested. Or is there something else that's lending itself to this shape?

[devin-ai-integration]

There's no good reason for this shape. All I needed was to stop a zero TTL from doubling as "no fallback", and a pointer does that just as well. I've switched to a pointer in 8ef7808: Fetch now returns *FDv1Fallback, which is nil when no fallback was requested, and FDv1Fallback only has TTL. A non-nil value with TTL: 0 means "fall back and use the default TTL". The internal PollingRequester.Request and the header/goodbye parsers use the same nil convention.

I left DataSynchronizerResult as FallbackToFDv1 bool plus an additive FDv1FallbackTTL. Folding both into a *FDv1Fallback would rename an existing field that a lot of code uses. I can do that here if you'd like it to match.

[beekld]

So now that this struct is just a single field, maybe we could just use a *time.Duration instead?

I left DataSynchronizerResult as FallbackToFDv1 bool plus an additive FDv1FallbackTTL. Folding both into a *FDv1Fallback would rename an existing field that a lot of code uses.

I think it would be better to remove the bool, so that we don't have two variables with depedendent values to keep in sync.

[devin-ai-integration]

Done in 0ffa24f. FDv1Fallback is gone. Fetch now returns fdv1FallbackTTL *time.Duration, and DataSynchronizerResult.FallbackToFDv1 bool plus the TTL field are replaced by a single FDv1FallbackTTL *time.Duration. In both places, nil means no fallback was requested, and a non-nil zero means "fall back and use the jittered default". interfaces.DataSynchronizerStatus.FallbackToFDv1 is a separate public status type that this PR doesn't otherwise touch, so I left it alone.

[beekld]

Now that the fdv1 fallback is a *time.Duration, it's not as self-documenting. Should we use named returned values to make the interface clearer?

[devin-ai-integration]

Yes, done in 0227d1b. PollingRequester.Request now names its results (changeSet, headers, fdv1FallbackTTL, err) and has a doc comment explaining what nil and zero mean for fdv1FallbackTTL. The two implementations (pollingRequester.Request and fdv1ToFDv2Requester.Request) use the same names. I left the private pollingRequester.request helper unnamed, because it already has a local changeSet builder that would clash with the result name.

[beekld]

It seems strange that if there's no fallback, then this case ends and we treat this as a generic "didn't receive any known protocol events" error. Should a nil fallback on a goodbye really be counted as an error? And if it is an error, then shouldn't it have a specific error message?

[devin-ai-integration]

A goodbye without a fallback TTL isn't treated as an error. The case ends, the for loop moves on to the next event, and a later payload-transferred still returns the change set normally. That's the same as before this PR, when the switch had no goodbye case and those events were skipped. You only get the generic "didn't receive any known protocol events" error if the payload ends without a payload-transferred. That happens whether or not it contained a goodbye, so a goodbye-specific message there would be misleading.

That said, silently skipping it was unclear. In 49a07eb the polling path now logs the goodbye reason at Info, as streaming does, and has a comment saying that a goodbye without a TTL is informational. I also added a goodbye without fallback TTL case to TestV2RequestorImplRequestAll. It checks that a payload with such a goodbye in the middle still returns its change set, with no error and a nil fallback TTL.

The same commit also fixes require.NotNil(t, *result.FDv1FallbackTTL) in the polling and streaming tests. It dereferenced the pointer before the nil check; it now checks the pointer itself.