feat: gate features by kill switch, release stage and time window (Stage 3) - #216
Conversation
The fat jar sets duplicatesStrategy = EXCLUDE, which keeps the first copy of every duplicate path and pre-empts mergeServiceFiles(). Any service file shipped by more than one jar therefore lost all but one set of entries: the packaged META-INF/services/org.togglz.core.spi.FeatureManagerProvider held Titan's provider alone, with Togglz's own five providers dropped silently. Let META-INF/services/** through as INCLUDE so the merge transformer sees every copy. Titan's provider keeps winning the lookup: it declares priority 30, the lowest of all registered providers, and Togglz asks the lowest first. This matters as soon as Titan registers an activation strategy of its own - without the fix, Titan's service file would shadow Togglz's built-in strategies.
Two building blocks for the staged delivery of US-3.01 to US-3.06. ReleaseStage names the three audiences a feature can be released to and answers who belongs to them: internal needs titan.feature.internal, lite additionally admits the LuckPerms group lite, ga admits everyone. It asks a FeatureAudience rather than LuckPerms directly, so :common stays free of LuckPerms types and the order of the checks is testable without a permission backend - the same rule TitanPermissionBridge follows for the CloudNet bridge. A missing or unknown stage resolves to internal, never to a wider audience. SeasonWindowActivationStrategy is the Togglz activation strategy that carries the time window. Togglz's own ReleaseDateActivationStrategy knows only PARAM_DATE and PARAM_TIME - a point in time after which a feature is on, with no end and no zone (verified with javap against togglz-core 4.6.2). A season needs both, so this one takes from, to and zone, each optional, and reads its time from an injected Clock. A parameter that is present but unreadable makes the feature inactive: a typo in a date must not widen an audience. Registered through META-INF/services/org.togglz.core.spi.ActivationStrategy, which Togglz's DefaultActivationStrategyProvider reads with a plain ServiceLoader.load(Class) - a call that uses the thread context classloader, the reason ThreadHelper exists. Tests cover the audience of each stage, both window bounds, one-sided windows, the zone parameter across a summer-time date, unreadable values, the service-file registration itself and a FeatureManager dispatching to the strategy it found there. TitanFeaturesTest holds the flag list to the ceiling of twelve (NFR-009).
FeatureGate is the single place that decides whether a player sees a feature, and
the only type in Titan that talks to Togglz. It walks the three steps of US-3.07
in the order the spec fixes:
1. kill switch - a disabled feature is invisible to everyone, whatever its
stage and window say. A feature that was never enabled counts as disabled,
so an unconfigured feature stays dark rather than going public.
2. release stage - read from the feature-state parameter "stage".
3. time window - evaluated by SeasonWindowActivationStrategy.
The three steps form a conjunction, so the order does not change the answer; it
decides which step is reported as the reason, which is what /season status shows
and what the tests pin down.
StageTransitionLogger covers US-3.09. Stages live in a flag file that is reloaded
in the background, so a stage change is not an event anyone fires - it is a
difference between two observations. The logger turns that difference into one
line with timestamp, old stage and new stage, and stays silent on the first
observation so a restart does not fake a transition. FeatureGate#pollStageTransitions
walks every feature once for callers that want to schedule the comparison.
Tests cover the order the spec cares about: the kill switch beating an open
window on a generally released feature, lite players seeing what ga has not
reached, a player without permissions seeing nothing outside ga, and the stage
being reported as the reason where both stage and window would deny. Added to
that: a missing and an unknown stage falling back to internal, an unconfigured
feature staying invisible, and a real flags.properties read through
FileBasedStateRepository.
LuckPermsFeatureAudience answers the gate's two questions through the permission system Titan already embeds: permissions from the user's cached permission data, group membership from the inherited groups of the user's query options, so a group that lite itself inherits from counts too. A player LuckPerms has not loaded yet holds nothing, which keeps an unfinished login on the narrow side of every release stage. SeasonCommand covers US-3.08. Togglz ships an admin console, but it is a servlet application and a Minestom process has no servlet container, so a command is what replaces it: /season status prints stage, window and kill switch per feature. The command is bound to titan.feature.internal - the same permission that defines the internal audience - and, like /stop, is always available from the server console. Titan now builds the gate from an injected Clock and ZoneId (NFR-007) defaulting to Europe/Berlin, registers the command, and schedules the stage comparison once a second so a transition is logged even while nobody is online. The commands package gets its package-info with @NotNullByDefault; the two @NotNull annotations it made redundant are removed.
The stage table listed premium/titan.feature.premium, which no longer matches US-3.02: the middle stage is lite, and membership is decided by the LuckPerms group lite rather than by a permission of its own. Corrected, and extended by what an operator actually needs: - the flags.properties keys behind kill switch, stage, window and zone, with the warning that a mid-line # is part of the value in a .properties file, not a comment - the shape of the log line the application writes on a stage change (US-3.09), and the note that the first look after a restart is not a transition - the pointer to /season status as the replacement for Togglz's servlet console Stage-3 stories US-3.01 to US-3.09 are marked as implemented in the spec, and the two acceptance criteria they satisfy are ticked.
Test results 87 files 87 suites 41s ⏱️ Results for commit 9f832e1. ♻️ This comment has been updated with latest results. |
An architecture review found that FeatureGate has zero production call sites. `isVisibleTo` and `decide` are consumed only by `/season status` and the stage-transition poll; NavigationHelper still sets all four items unconditionally, and `TitanFeatures.isActive()` now has no callers at all. So the mechanism is built and tested, but nothing asks it anything. An operator writing `NAVIGATOR_ELYTRA = false` gets a `/season status` that reports the kill switch as engaged while every player keeps seeing and clicking the item. US-3.01 through 3.04 and 3.06 were marked `umgesetzt` and two acceptance boxes were ticked. They are none of those things. Reverting the claims rather than the code - the gate itself is sound and worth keeping; it just needs wiring. US-3.08 is downgraded to partial: when `from`, `to` or `zone` fail to parse, the strategy returns empty and the command prints "window always" while the gate denies everyone. The one command whose purpose is to save the operator a trip to the log tells them the opposite of the truth. Not a regression: the unconditional item wiring predates this branch.
Architecture review — converting to draftA review of this branch found that the gate has zero production call sites. Concrete failure: an operator writes US-3.01, 3.02, 3.03, 3.04 and 3.06 are not met. They were marked Also downgraded: US-3.08. What stands upThe review confirmed the parts worth keeping, and they are substantial:
Two follow-ups, tracked separately
Returned to the implementing agent for wiring. |
The gate had no production call sites. NavigationHelper wrote all four destinations unconditionally, so an operator who set NAVIGATOR_ELYTRA = false got a /season status that reported the kill switch as engaged while every player kept seeing and clicking the item. US-3.01 to US-3.04 and US-3.06 were built and tested but never reached a player. Each destination is now asked of the gate before it is written into the layout. A denied entry is simply not written, so its slot keeps the filler pane that the whole row was filled with a line earlier; five of the nine slots are that pane in the normal case, so a hidden entry does not read as a hole. The entries keep their fixed slots rather than compacting: Stage 5 replaces this slot arithmetic with NavigatorLayout.plan(...), which filters first and then derives contiguous centred slots, and inventing a second layout mechanism here would mean throwing one of them away. The gate check is written as a plain per-entry predicate, which is the shape that folds into that filter. The ThreadLocalUserProvider.bind/release bracket and its toUser helper are gone. They fed a Togglz user to a gate that no longer exists; nothing between them read the bound user. Tests assert what a player actually sees: the navigator is opened, the server is ticked (Aves applies the data layout on the next tick, so asserting without the tick reads an empty inventory and passes for the wrong reason), and the slot's material is checked. Covered: all four destinations visible on ga, the kill switch replacing the elytra item with the filler pane while its neighbours keep their slots, an internal destination hidden from an ordinary player and shown to a holder of titan.feature.internal, a lite destination shown to the lite group only, and a flag flipped between two opens taking effect on the second. Four of those fail if the gate check is replaced by a constant true - the property the previous green build did not have.
…ng it
FeatureStatus could report hasWindow() == false and withinWindow() == false at
the same time, which cannot both be true. The cause: the strategy parsed the
window parameters twice, once failing closed for the decision and once quietly
returning empty for the display. A feature whose `from` was a typo therefore
denied everyone while /season status printed "window always" - the one command
whose purpose is to spare the operator a trip to the log told them the opposite
of the truth.
The strategy now reads the three parameters in one place that throws with a
message naming the offending parameter and its value; isWithinWindow catches it
to fail closed, and the new windowProblem() hands the same message to the status.
An unknown stage id is carried too: the gate falls back to internal, but the
operator who wrote `stage = intern` needs to see the typo rather than a stage
they did not configure - and the spec's own German checklist ("auf intern, lite
und ga stellen") is exactly what tempts them into writing it.
FeatureStatus rejects the contradiction in its compact constructor, so an
unreadable window can no longer be reported as an open one anywhere.
/season status prints "window unreadable: from='1. Oktober' is not a date ..."
and "stage internal ('intern' is not internal, lite or ga)" in red. Tests assert
that the broken window prints "unreadable", does not print "always", and names
the offending value, plus the gate-level status fields and the constructor guard.
US-3.01 to US-3.04 and US-3.06 are marked `umgesetzt (Navigator)` - qualified by surface on purpose. The navigator is the only player-facing surface that exists today; seasonal content and portals will have to consult the gate themselves when they arrive, and the qualifier keeps that visible instead of implying the whole lobby is covered. US-3.08 goes back to `umgesetzt`: the command no longer contradicts the gate on unreadable configuration. The rollout log gains what that looks like, so an operator who mistypes a date recognises the output as a pointer to the typo. The two acceptance boxes are ticked against tests rather than intent: stepping a feature through internal, lite and ga is covered by NavigationHelperTest, and the kill switch beating stage and window by FeatureGateTest, with the reload interval carrying the two-second requirement.
The gate is now wired into the navigator and the criterion is met, but not without a boundary worth writing down: the check happens when the menu is drawn. A player who already has the navigator open keeps the old picture until the next open. That window is narrow - a denied entry gets no click handler and InventoryPreClickEvent is cancelled globally - but it is real, and a click-time re-check belongs on Stage 5's NavigatorEntry, which already carries the permission an entry requires.
Wired — review findings addressedThe blocker is fixed. if (isVisible(TitanFeatures.NAVIGATOR_ELYTRA, player)) {
finalLayout.setItem(0, Items.NAVIGATOR_ELYTRA_ITEM_STACK, this::clickElytra);
}The dead I verified the tests actually biteThe previous round passed a green build while the gate was never called, so I did not take "tests are green" for an answer this time. Mutation check: replacing The implementing agent found the reason the first attempt would have fooled us: Aves applies the data layout on the next tick, so an assertion made straight after US-3.08 fixed at the root
Scope kept deliberately smallStage 5 (stacked on this branch) had already replaced the hardcoded slot writes with a Known boundary, written into the specThe check is display-time. A player holding the navigator open when the kill switch flips still sees the stale item until the next open. A denied entry gets no click handler and Still open, tracked elsewhere
|
TitanFeatures and SingletonFeatureManagerProvider stayed in common/utils when common/feature was created, and a new test was added next to them there. common/utils is the catch-all package OLF-L3-02 names as the anti-pattern, and it lists both classes by name with common/feature as their destination. Move both plus TitanFeaturesTest, and point the FeatureManagerProvider service file at the new name. Callers only change their import. ThreadHelper stays in common/utils: it is a cross-project duplicate whose destination is Butterfly (OLF-L2-04, open point 4), not another Titan package. Its javadoc now says so.
…le order Since META-INF/services/** is merged into the fat jar, three copies of org.togglz.core.spi.FeatureManagerProvider end up in one file and Butterfly's SingletonFeatureManagerProvider is registered for the first time. It declared priority 30 - the same value Titan declared - and reads the same flags.properties, but builds its manager from ButterflyFeatures. Togglz sorts providers by priority ascending with List.sort, which is stable, so Titan won only because shadow's classpath walk happened to list it first. Reordering a dependency in app/build.gradle.kts or switching implementation to api flips it, and the failure is partial: getFeatureState still resolves Titan's flags by name while statuses(), pollStageTransitions() and /season status enumerate Butterfly's enum. Drop Titan's priority to 10, below Butterfly's 30 and below Togglz's own providers (50-200), and record the reasoning plus the OLF-L2-05 note on the static manager field in the class javadoc. FeatureManagerProviderResolutionTest, in :app because that is where both providers share a classpath, asserts the ambient manager enumerates TitanFeatures, that Titan's priority is strictly lower than every rival, and that Butterfly's provider is actually present so the first assertion cannot pass vacuously. Also narrow the shadowJar filesMatching: META-INF/services/** is broader than ServiceFileTransformer's own pattern set, which excludes the legacy Groovy extension descriptor. Nothing on the classpath ships one today, so guard that single path explicitly instead of widening the rule.
… context The lobby gave two answers to the same question. SeasonCommand.canUse goes through PermissionChecker.POINTER to TitanPlayer, which evaluates against the contextual query options LuckPerms resolves for the online player. LuckPermsFeatureAudience read User#getQueryOptions() instead - the holder's stored options, which carry no server, world or dimension context. A team member holding titan.feature.internal scoped to server=titan-lobby-1 was therefore inside the internal audience for /season and outside it for every feature the gate decides on. Answer a permission from the online player's own PermissionChecker - on a Titan lobby that is the TitanPlayer the command already consults - so the two answers cannot drift apart (OLF-L2-04). Group membership has no equivalent on the player object and stays on LuckPerms, but resolves against ContextManager#getQueryOptions(User), the same contextual options TitanPlayer uses; an offline player falls back to the static options. Second, LuckPermsProvider.get() throws NotLoadedException while LuckPerms is loading or after it failed to load, and nothing caught it even though the gate is consulted on every navigator open. Every answer now fails closed to false - the behaviour FeatureAudience.denyAll() documents as the safe default - and the outage is logged once rather than once per entry. Closed is the right direction because the stages only widen: failing open would promote every internal and lite feature to the whole server at the one moment nobody can revoke it, while failing closed only hides unreleased work. Stage ga admits everyone without consulting an audience, so an outage never hides the lobby itself. LuckPermsFeatureAudienceTest pins both: the stored-options regression fails two of its cases, and the outage cases assert that internal and lite deny while ga still admits. It needs the LuckPerms API on the test classpath, which compileOnly does not provide.
Residual review findings fixed — 114 tests greenOne answer to the permission question
Permissions now go through the online player's Backend outage fails closed, and the reasoning is worth recording: the stages only ever widen. Failing open would promote every Crucially this is not a lobby-wide blackout: The provider tie is goneConfirmed from the artifacts: Butterfly 1.0.23 ships its provider at priority 30 — identical to Titan's — and Togglz's Titan's provider now declares priority 10, below Butterfly and below every Togglz built-in (50/60/70/100/200). The new test has three assertions, and the middle one is load-bearing: (1) The Groovy extension descriptor is now guarded, not just documented: OLF-L3-02 violation cleared
|
1 participant
Stacked on #214, parallel to #215. Implements spec Stage 3 — US-3.01 … US-3.09.
What changed
A single
FeatureGatein:commonis now the only type that talks to Togglz. It walks US-3.07's order literally and returns aFeatureDecisionthat names the step which denied:internal→lite→gaSeasonWindowActivationStrategysupplies what Togglz cannot: its built-inReleaseDateActivationStrategyhas onlyPARAM_DATEandPARAM_TIME— no end, no zone. Ours takesfrom/to/zone, reads time from an injectedClock, and fails closed on an unparseable value.FeatureAudiencekeeps:commonfree of LuckPerms types by asking two JDK-typed questions (hasPermission,inGroup) — the same seamTitanPermissionBridgealready uses. The only production implementation delegates to LuckPerms; this is not a second permission system.Also:
/season statusfortitan.feature.internalholders, because Togglz's admin console is servlet-based and unusable in a Minestom process.A pre-existing bug this uncovered
Verifying that the SPI registration survives into the shipped jar exposed a real defect:
shadowJarsetsduplicatesStrategy = EXCLUDE, which keeps the first copy of a duplicate path and pre-emptsmergeServiceFiles(). The shippedapp-titan.jar'sorg.togglz.core.spi.FeatureManagerProvidertherefore contained Titan's provider alone, with Togglz's five silently dropped. The new activation strategy would have shadowed Togglz's built-ins the same way.Fixed by letting
META-INF/services/**through asINCLUDEso the transformer sees every copy. Titan's provider still wins the lookup: it declares priority 30, the lowest registered, and Togglz asks lowest first.Tests — 92 green (57 in
:common, 35 in:app)The evaluation order is what is actually worth testing, so it is pinned directly: with an internal stage and a closed window, an anonymous player is denied by stage and a team member by window. Plus kill switch beating an open window,
liteseeing whatgahas not reached, unknown stage falling back tointernal(never wider), a realflags.propertiesread throughFileBasedStateRepository, andTitanFeaturesTestfailing the build above twelve constants (NFR-009).The SPI registration is verified three ways, not just compiled:
ServiceLoaderreturns it; a realFeatureManagerwith the productionDefaultActivationStrategyProviderdispatches to it by id; and the built jar's merged service file lists it alongside Togglz's seven built-ins.Not verified
/season status, the 1-second scheduler task andLuckPermsFeatureAudiencewere never exercised against a running Minestom + LuckPerms process. The audience class has no test at all — the LuckPerms API iscompileOnlyand its loader is deliberately off the test classpath, so those lookups are compile-verified only.FileBasedStateRepository(File)already delegates with a 1000 ms interval, confirmed by bytecode).One correction to the docs
rollout-log.mdcalled the middle stagepremiumwith a permission, contradicting US-3.02 (lite, a LuckPerms group). The spec won; the document is corrected, including theflags.propertieskeys an operator needs and a warning that a mid-line#in a properties file is part of the value, not a comment.