| Age | Commit message (Collapse) | Author |
|---|
|
The tagline and both plugin descriptors now say that romaji conversion can run
inside the server through anthy, so the engine is visible wherever the project
describes itself: the README, the website hero and meta description, the API
documentation, and the Paper and Velocity plugin descriptors.
|
|
Add a second engine for romaji-to-Japanese conversion that leaves the process
alone: anthy, compiled to a WebAssembly module and executed in the JVM by
Chicory's build-time AOT, so conversion needs no outbound request.
anthy/ carries the module sources, the WASI shims, the reproducible build
script, the committed AOT output and the dictionary (~23 MB, extracted into
the plugin data folder on first start; an existing file is left alone).
features.japaneseConversion.engine selects "google" or "anthy", and the anthy
block adds dictionary, workDirectory and sentenceLevel.
RomanjiConverter now sits on a KanjiConverter interface, so a whole message is
handed to the engine at once - anthy segments and reorders a sentence instead
of converting word by word - and the conversion cache is keyed by that
sentence. The engine tests drive the real module, the dictionary tests cover
the extraction, and everything else keeps running on the existing fakes.
|
|
{world} rendered empty for a direct message from another server, which
left formats like "({world})" showing a bare pair of brackets. The relay
now carries the sender's world as an optional field, so this is a PATCH
bump (protocol 1.0.2) and either side can be updated first; an older
sender or proxy still yields an empty value.
The world is read once per message, so the sender, the spies and the
relayed copy can no longer disagree if the sender changes worlds mid-way.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Serializing the display name into the legacy text let its color run into
everything after it, and legacy text cannot hold hex colors or hover and
click events, so names styled by other plugins came out wrong. The name is
now spliced into the parsed component instead.
{role} is resolved only when the format uses it, since the default
formats do not and it costs a member scan and a lookup per message.
/lc channel status now shows the same localized role names instead of a
second, hard-coded [OWNER] / [MOD] vocabulary that only one of the two
places translated.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Operators asked for more of the sender's context in chat formats than the
name alone. {world} covers direct messages and channel chat; the channel
details and the sender's display name and role only exist for channel
chat, so only that format accepts them.
A direct message from another server renders {world} empty: the relay does
not carry the sender's world, and adding it would need a protocol bump and
a coordinated Velocity release for one placeholder. The renderer now
accepts underscores so the snake_case names match as written.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Placeholder names were spelled as string literals in three handlers, and
each handler also chose its own way of turning the result into a
Component. Adding a placeholder meant finding every call site by hand, with
nothing to catch a format left without its value.
ChatPlaceholder now owns the names, and each format has an entry point whose
parameters are exactly the placeholders it accepts, so the compiler points
at the callers when one is added. Substitution happens in a single pass, so
a value that itself contains "{message}" is no longer replaced again by the
next placeholder. All three formats now deserialize section signs into
styles, which leaves what players see unchanged.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
The update checker read releases/latest and stripped a leading "v", so any
release whose tag was not a bare vX.Y.Z (paper/v1.2.x, or a Velocity release
published last) parsed as 0.x and no update was ever reported. It now lists
recent releases and picks the highest Paper tag, skipping drafts,
prereleases and non-Paper tags, so a Velocity-only release can no longer
hide a Paper update.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Three findings from review of #296.
Brigadier evaluates a `requires` predicate when it builds the command tree it
sends to a player - on every join and every updateCommands() - not when someone
is refused. The denial line sitting there put one entry per gated node per
player in the log, burying the command results the category exists to show. It
is dropped rather than moved: a hidden node is never reached by an execution, so
there is no refusal path to log from.
`all` was honoured by `debug: all` and `/lc debug all on` but not by
`categories: [all]`, which resolved to an unknown name - leaving `enabled: true`
with nothing switched on, reported as off by /lc status, explained only by a
warning in the log. The word now lives in one place that all three read.
Two dumps in the same second left one file, because the name is only accurate to
the second and the write replaces. Reproducing a bug means dumping either side of
the step that triggers it, so the second one now takes a suffix.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
The reply carried the report's path so the operator could find the file. But
`lunaticchat.command.lc.dump` can be granted to someone with no filesystem access
at all, and to them a path is not something they can act on - it only says how
the server is laid out. Relativising it against the server directory narrowed
that without removing it.
Naming the folder in the message says everything a reader can use, and leaves the
exact path to the server log, which only someone with that access can read
anyway. That also makes the reply a single message rather than a message plus a
component the language files could not translate.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
|
|
config.yml now accepts `debug: velocity,protocol` and a block with `enabled` and
`categories` beside the boolean it always took. Categories exist so that
instrumenting the chat path does not bury the handshake an operator is chasing
under one line per message.
The setting is read by inspecting the YAML node by hand rather than through a
data class, because one key has to accept both a scalar and a map and kaml picks
the input to build from the descriptor kind alone - a class descriptor rejects
`debug: true` before the serializer is ever called. An unknown category name is
collected rather than thrown: throwing would make ConfigManager prune the whole
`debug` key and switch off the categories the operator did spell correctly.
`debug` moves from RESTART_REQUIRED to applied, since a reload only has to
replace a DebugState. `/lc debug` switches a category for the running server and
deliberately does not write config.yml back - re-emitting a commented YAML
document from a parsed tree loses every comment and every bit of formatting the
operator put there, and a restart or `/lc reload` putting the file back in charge
is the safer default anyway.
`/lc dump` writes the report a bug report needs: versions, enabled features,
connection state and how full each store is. It carries no message text, player
name or UUID, because it exists to be pasted into a public issue by someone who
is not reading it first. Its reply names the file relative to the server
directory - the command can be granted to someone with no filesystem access, to
whom an absolute path discloses where the server is installed and nothing they
can act on.
The KtorSimpleLogger calls in ChannelMessageHandler and ChannelMessageLogger are
folded into the same logger, so there is one place that decides whether a
diagnostic line is printed.
Refs #228
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
"Debug" had grown into three unconnected mechanisms: a boolean injected into
RomanjiConverter, a one-off configuration dump at startup, and KtorSimpleLogger
calls that Paper's stock log4j configuration never printed. Turning `debug: true`
on told an operator nothing about the Velocity handshake, which is what #228 is
actually about.
The vocabulary goes in engine because it is a contract before it is an
implementation: config.yml and the proxy's system property have to agree on what
`velocity,protocol` means, and a category added on one side must exist on the
other. Only the writing is left to the platforms.
The message is a lambda so that the chat path - instrumented once per message -
costs nothing while debugging is off. The `if (debugMode) logger.info("... $word")`
guards this replaces built their strings either way.
Both platform loggers write at INFO rather than at the level the content
deserves, because neither Paper's nor Velocity's stock logging configuration
prints anything below it, and asking operators to edit log4j before they can
answer a bug report is the friction this exists to remove.
Refs #228
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
A nightly JAR was named after the bare release version, so a build from
main and the release of the same base version were indistinguishable in
the JAR file name, /plugins and bug reports; only build-info.properties
knew the difference, and Velocity does not even ship that file.
The x script tried to express this with -Pversion, but each module
assigns its own version from paperVersion/velocityVersion, so the
override never took effect.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
It shows up as an uncovered line on every patch that touches parse(),
and the next reader would otherwise have to rediscover that no config.yml
can reach it.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
|
|
|
|
The Japanese wording was revised in place: the messages now say "the
configuration file" rather than naming config.yml at the operator, and they
follow the punctuation this file already used for in-game text. English is
brought in line with that wording, keeping its own trailing periods, which
the rest of en.yml uses on every full sentence.
The three failure headings now carry a comment saying what is printed
beneath them and why those lines are never translated. A translator has no
way to see that from the file alone, and the two reasons it cannot change
are both easy to undo by accident: the text comes from the YAML parser, so a
parser update would silently age any translation kept here, and it cannot be
passed as a placeholder because a reason holding braces of its own is broken
by the next substitution and then coloured as if the braces were ours.
|
|
The reply listed every setting that had moved and every setting that still
needed a restart, as dotted keys. That buried the one bit that changes what
the operator does next - whether they have to restart - under names they had
just typed themselves and did not need read back to them.
The detail is not lost: which settings moved was already written to the
server log on every reload, which is the better home for it anyway, since a
format change can be dated there long after the chat line has scrolled away.
A pending restart now outranks "nothing changed" even when nothing could be
applied. Reporting no change to someone who did edit the file would send
them hunting for a typo they had not made.
The failure path still names each unreadable setting and the reason it was
rejected, because that is a list the operator cannot reconstruct and needs
in order to fix the file.
|
|
Changing a message format meant taking the server down, because config.yml
was read once in onEnable and every consumer held the value it was built
with. Operators tune these formats far more often than anything else in the
file, and a restart is a disproportionate price for a colour code.
Only messageFormat is applied. The rest of config.yml settles things once,
at startup - which services exist, which commands and listeners are
registered, which files are opened - and none of that can be redone while
the server runs: Paper registers commands only through its COMMANDS
lifecycle event, Bukkit cannot unregister a listener, and the conversion
cache saver keeps no task handle to cancel. Rebuilding the services would
leave the old ones still wired up, so a changed setting the server cannot
pick up is reported back as needing a restart rather than silently ignored.
MessageFormatHolder is deliberately narrower than LunaticChatConfiguration:
taking it says the value is live, taking the configuration says it was
frozen at startup. A holder over the whole tree would hold a configuration
matching no version of the file, since only messageFormat is ever swapped.
Reload reads config.yml more strictly than startup does, because it has an
option startup does not - leaving the running configuration alone. Startup
falls a single unreadable setting back to its default so the server can come
up; reload refuses the file, names every unreadable setting at once, and
keeps what the server already had. A document holding no settings is refused
for the same reason: it cannot be told apart from a config.yml caught
halfway through being written, and resetting every format over that is worse
than doing nothing.
Closes #266
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Making a conversion timeout an ordinary exception put it in the same arm as a
hard API failure, where caching the hiragana fallback is deliberate - so one slow
reply recorded the unconverted form and that word rendered as hiragana for the
life of the cache. A timeout says the request was slow, not that the word has no
conversion, so it now returns the fallback without caching it and the next
message asks again.
The retry test builds its own remembering cache: the shared fixture's get()
always returns null, so against it the API is called every time and the test
would have passed even with the timeout cached.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
- getDirectMessageSpyPlayers had no callers left; handing out a copy of the spy
map is the shape notifySpies was introduced to replace.
- pluginScope was widened to public for "commands that must not block the tick
thread", but commands take the delivery queue and only LunaticChat reads it.
- getPlayerChannels returned a Result that cannot fail, so three callers carried
unreachable error paths and channel.status.error could never be shown.
- The cross-server managers were gated on velocityIntegration.enabled as well as
on a manager that is non-null only when it is enabled, letting the two
conditions disagree.
- LenientBoolean's non-YamlInput fallback was observationally identical to the
cast failing, since both land in ConfigManager's catch-all.
- sendCrossServerMessage caught its own failures underneath the delivery queue,
which already reports them without stopping the sender's later messages. The
second boundary is what forced a CancellationException clause here.
- handleOutgoingCrossServerMessage still recorded the reply target after the
commands took that over, so remote targets were recorded twice - re-inserting
entries clearPlayer had swept, which is the bug the local path was fixed for.
- The reason delivery is queued was written out in both command constructors and
twice more in KDoc; it now lives where the queueing happens.
Spy notification also defers its notice lookup and member set until a spy is
actually online, which is not the normal case.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Atomicity was opt-in per write site, so a file added later was safe only if its
author noticed the convention. Worse, DebouncedSaver drops a request while one is
pending and so serves exactly one file - a rule held up only by the wiring
happening to construct a separate saver per file, and written down nowhere. A
FileStore now owns its file, its atomic write and its own saver, so neither can
be got wrong by wiring; writeTextAtomically is internal to the package.
Taking the Bukkit plugin out of DebouncedSaver in favour of an AsyncScheduler
makes the debounce testable at all: ChannelStorage's "the snapshot is taken when
the write runs" now runs against the real thing rather than a mocked saver.
Teardown gets the same treatment. The five services with shutdown work spelled it
saveToDisk() three times and shutdown() twice, and the list of them was
hand-maintained against a fourteen-field container - so a new service was not
stopped unless someone remembered a second place. They now implement
StoppableService and register as they are built, and shutdown iterates that list.
stop() delegates rather than renames, because the conversion cache's periodic
flush is a different caller from shutdown.
Also here, on files this commit already touches: player settings carry a dirty
flag, since updateSettings is the only writer and queues its own save, so every
quit was re-serializing every stored player to write identical bytes; and
setPlayerChannel returns early when nothing moved, because the quit path clears
the active channel for every player whether or not they had one, and a mass
disconnect paid a full snapshot per player in one tick.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
An item takes up to the conversion timeout to drain, far slower than a player
can send, so an unbounded queue grew for as long as a macro ran - each item
holding its sender and recipient alive and arriving minutes after it was typed.
Refusing the overflow is at least visible to the player.
Releasing a player now cancels their worker instead of letting the backlog run.
Finishing it would spend a round trip per item writing to somebody who has left,
and keep both players reachable until it drained.
Chosen over DROP_LATEST on the channel: that reports success to trySend and
drops silently, which would make the warning about discarded work unreachable in
the case it was written for.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
withTimeout reports a timeout as a CancellationException, which callers must
rethrow rather than degrade. With api.timeout below convertWithRomaji's budget
the client's timeout therefore travelled through convertWord, through
withTimeoutOrNull - which rethrows a timeout belonging to another coroutine -
and into the queue worker, which read it as shutdown and ended its loop. The
channel stayed registered with nothing reading it, so every later message from
that player was buffered and never delivered: exactly the failure the worker's
own guard exists to prevent. A timeout is now an ordinary exception, so
cancellation once again means only cancellation.
The concurrency limiter also no longer covers the cache lookup. Its four
permits are shared by the whole server and held for a full round trip, so
cached words queued behind in-flight requests for a permit they did not need
and a message whose every word was cached could still exhaust the caller's
budget and go out unconverted.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Catching YamlException at the parse step covered kaml but not the scanner
underneath it: a file saved as UTF-16, or one truncated with NUL padding after an
unclean shutdown, fails inside snakeyaml-engine's reader with an exception that
is not a YamlException. It escaped onEnable and Bukkit disabled the plugin over a
config file - the very failure reading per setting was meant to prevent, and one
the catch-all this replaced had handled. The same gap swallowed the serializer's
own non-YamlException fallback.
A UTF-8 BOM is also stripped before parsing. It otherwise stays on the first key,
which strictMode = false drops as an unknown setting without logging anything, so
the operator sees exactly one setting ignored and no reason why.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
kaml rejects a document as a whole, so one unreadable value lost every other
setting the operator had written: velocityIntegration off, serverName "Unknown"
and channel chat disabled, behind one vague SEVERE line. The hand-written mapper
this replaced defaulted per key, so reading the file directly had quietly made
config.yml far more brittle than it was.
The offending setting is now dropped by the path its parse error carries and the
rest of the document is decoded, so the operator loses the one value they got
wrong and is told which. Only a document that is not YAML at all still costs
them everything.
Booleans also accept the YAML 1.1 spellings again. Bukkit read config.yml as
YAML 1.1, where `yes`, `no`, `on` and `off` are booleans; under kaml's YAML 1.2
they became strings, so `checkForUpdates: no` would have reset to its default -
which is the opposite of what the file says.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
readText sat outside ConfigManager, so the documented "fall back to defaults"
never covered the read itself. saveDefaultConfig only logs when it fails to
write the file, so the read can still find nothing there - and the IOException
then escaped onEnable and Paper disabled the plugin outright.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
CommandResult carried Adventure Components, which was engine's last
Minecraft dependency and the reason CLAUDE.md's "engine has no Minecraft
platform dependencies" was not quite true. It also meant a command could
not report a result without having already decided how it looks: every
site had to pick formatError versus format before it could return.
Results now carry text, and LunaticCommandBase.handleResult is the single
place that styles it - error red for Failure, normal for
SuccessWithMessage. The fail()/ok() helpers from #260 already funnelled
every call site through two functions, so this is a change to those two
plus the one command that composes its own success text.
engine's dependency list is down to kotlinx-serialization, and nothing
under engine/src references net.kyori, org.bukkit, com.velocitypowered or
io.papermc.
Not done: the review also proposed collapsing the per-command
`when (error)` blocks into one exception-to-key table. Those blocks pick
wording, not just a key - "only owners can delete this channel" reads
differently in the ban command than the delete command - so a shared table
would hand every caller the same sentence and need per-command overrides
on top. Left alone deliberately.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Every setting's default was written three times - in config.yml, in the
ConfigManager getter call, and on the data class - and copying the file
key by key is what made that necessary. They had already drifted:
checkForUpdates defaulted to false in ConfigManager while both config.yml
and the data class said true.
Worse, features.channelChat.messageLogging was documented in config.yml
with three settings and never parsed at all. ConfigManager did not build
it, so ChannelMessageLoggingConfig() always won and an operator editing
retentionDays or maxFileSizeMB changed nothing. Those settings now take
effect - the documented behaviour, but a real change for anyone whose file
disagrees with the defaults.
KAML deserializes the file straight into the tree, the same way player
settings and channel data are already read, so a default now lives only on
the data class. Two consequences worth stating:
- japaneseConversion.cache and .api are nested classes now, because the
data has to match the file rather than the file being flattened by hand
on the way in. The YAML is unchanged.
- api.retryAttempts is gone from config.yml. It was parsed and stored, but
never reached GoogleIMEClient or RomanjiConverter, so it documented a
knob that did nothing.
Unknown keys are ignored and a malformed file falls back to defaults with
a log line, so neither an old config nor a typo stops the server booting.
The tests parse real YAML instead of a mocked FileConfiguration, which
lets them cover what the mock could not: a partial file, a retired key, a
malformed document, and - the one that would have caught the drift above -
that the bundled config.yml equals the declared defaults.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
DirectMessageError.reason was a String backed by two constants, so the
receiving side matched one case and let everything else fall through to
"the target is offline". Adding a third reason on the proxy would have
shipped it to Paper servers that silently reported the wrong thing - the
one string-keyed dispatch sitting next to a protocol layer whose messages
are otherwise a sealed hierarchy with exhaustiveness checking.
As an enum, the reader must decide what to show for each case, and
CrossServerDirectMessageManager's when no longer needs an else.
The wire format is unchanged: kotlinx serializes an enum as its name, so
the existing snapshots still decode. What did need care is the reverse
direction - a reason from a newer proxy would now fail to parse, where the
String version degraded. The property has a default and the codec enables
coerceInputValues, so an unknown reason lands on TARGET_OFFLINE, exactly
the old else branch. There is a compatibility test for that case.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Closes #259.
engine exposed ktor through api(), so both platforms inherited the client
and its CIO engine. The only thing in engine that used ktor was
GoogleIMEClient, and the only module that used GoogleIMEClient was
platform-paper - Velocity was shipping roughly six megabytes of HTTP
client to support a Paper-only feature. Same story for
kotlinx-coroutines-core, which Velocity does not use at all.
Romaji conversion is a Paper feature, so the converter package now lives
in platform-paper alongside the ConversionCache and RomanjiConverter that
were already there. engine keeps kotlinx-serialization on api(), which is
genuine shared surface: the plugin messaging protocol is built on it.
The velocity shadow jar goes from 7,618,405 to 2,769,395 bytes, and no
longer contains io/ktor at all.
CacheData's tests were sitting inside engine's SettingsDataClassesTest,
which is unrelated to settings; they move with the class.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Both commands already record the conversation on the command thread before
queueing the delivery, for the same reason recordRemoteRecipient exists: /reply
reads the target there. Recording it again inside the queued work was not only
redundant but late, re-inserting entries that clearPlayer had already swept - so
lastMessager grew by one dead UUID every time a recipient quit mid-delivery.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Cancelling the scope kills the worker coroutines but does not close their
channels, so trySend kept reporting success for work nothing would ever read.
The warning that exists precisely to avoid dropping a message in silence was
therefore unreachable in the case it was written for.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
The catch-all around the Google IME call also caught the CancellationException
from the caller's timeout, and then cached the unconverted hiragana. Since the
words of a message are now converted concurrently, one timeout pinned every
word of that message to its hiragana form for the life of the cache instead of
just the word that timed out.
The same swallowing let a cancelled delivery carry on past the plugin scope
being cancelled, and reported the shutdown of an in-flight cross-server
message as a SEVERE delivery failure.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
The steps ran as one statement each, so the first exception escaped
onDisable and took the rest with it - leaving the channel message logger
unflushed and the Velocity connection to be torn down by the server rather
than by us. Each step is independent, so a failure is now reported and the
remaining ones still run.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Writing to a fixed sibling only moved the interleaving from the destination
to the temporary file: two saves racing there published mixed content, and
the losing move then failed with the temporary file already gone. Each write
now gets a unique temporary file, and falls back to a non-atomic replace on
the network mounts that refuse an atomic rename.
settings.yml and the conversion cache were still written in place. Both are
discarded wholesale when they do not parse, so a torn file silently costs
every player's settings or the whole accumulated cache.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Findings from reviewing the branch, in descending order of how much they
could hurt.
A message that threw ended the consumer loop while its channel stayed
registered in the map. Every later message from that player was then
buffered with nothing reading it, so their /tell and /reply stopped working
silently until they reconnected - one bad message format was enough. The
CoroutineExceptionHandler added earlier reported the throw but could not
bring the worker back. Each item now runs inside its own guard, so a failure
costs one message instead of the player's whole session.
Reply targets were recorded inside the queued delivery, but /reply reads
them on the command thread. /tell followed straight by /r therefore failed
with "nobody to reply to" for as long as the conversion took - up to the
full timeout. They are recorded before the work is queued now, which is
also where the sender expects the effect to happen.
Three narrower ones:
- A refused trySend was discarded without a word. It can happen once the
scope is cancelled at shutdown, and dropping a player's message in
silence is the worst way to handle it.
- Parallel conversion opened one API request per word with no ceiling, and
a repeated word in one line no longer shared the cache - the sequential
version got that for free by caching before the next lookup. Rate-limited
replies land in convertWord's catch and quietly degrade to hiragana, so
it is better not to ask that hard: distinct words only, four at a time.
- coerceAtLeast(1) on the cache interval turned a configured 0 into a whole
file rewrite every second, since any chat re-arms the dirty flag. A
non-positive value now falls back to the documented 300 with a warning.
ChannelStorage writes through a temporary file and an atomic move. Bukkit
runs onDisable before cancelling scheduler tasks, so the shutdown save can
overlap a still-pending debounced save, and two truncating writes to
channels.json would interleave into something unparseable. Debouncing made
that window much wider than the old runNow did.
TestLogger now captures log(level, msg, thrown); it only overrode
severe/warning/info, which do not route through each other, so anything
logged with a throwable attached was invisible to every assertion.
The two queue tests fail against the unguarded loop.
Left as review comments: delivery in flight can re-add a reply entry for a
player who just quit. resolveValidTarget filters unreachable targets on
read and the entry is cleared on their next quit, so the cost is a lingering
map entry, not wrong behaviour - and removing it would change what
sendDirectMessage promises, which a test pins.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Passing ::snapshot to the debounced write meant the three caches were read
from the writer thread while the server thread mutated them. They are
separate maps, so a snapshot could catch a channel already removed from
channelsCache while its membersCache entry still existed - persisting an
orphaned member list - or the reverse, persisting a channel with no members
and therefore no owner. Neither crashes anything, since the readers filter
on channelsCache, but they are wrong state written to channels.json and
carried across restarts.
The snapshot is taken on the mutating thread again and handed over through
a volatile field, so the write still reads the newest state when it
eventually runs rather than the state at queue time. That keeps both
properties: consistent halves, and a batched write that reflects every
change made during the delay.
This does not give back the per-change cost the earlier commit was avoiding,
because copying three maps is not what made saving expensive - the debounce
already coalesces the file write, which is.
Also floors the conversion cache interval at one second. It is now the only
writer besides shutdown, so a non-positive value would both be rejected by
runAtFixedRate and leave the cache unsaved until the server stopped.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Two regressions from taking the direct message path off the tick thread in
this branch.
A coroutine that threw reported to the JVM default handler, so nothing
reached the plugin log - and since the command had already returned
Success, a failed delivery was invisible to the player and the operator
alike. Before the change the exception propagated out of execute() into
Brigadier. PluginCoroutineScope now carries a CoroutineExceptionHandler,
which every caller that dispatches and returns depends on.
Launching a coroutine per message also dropped the ordering the synchronous
version had. Dispatchers.Default is a pool, so a cached romaji conversion
finishing in microseconds could overtake an uncached one sent before it:
the sender sees their second message first, and on the cross-server path
the relay to Velocity inverts too, so the recipient does as well. With
Japanese conversion on, mixing cached and uncached words is ordinary, not
an edge case.
A mutex would not have fixed it - two launched coroutines reach the lock in
whatever order the pool starts them. PerPlayerWorkQueue instead gives each
player a channel with a single consumer, so submission order is decided on
the calling thread and preserved, while different players stay independent.
The queue is dropped when a player quits; work already queued still runs.
The ordering test fails against the previous launch-per-message code.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
/tell and /reply ran romaji conversion inline. Brigadier executors run on
the main thread, and convertWithRomaji wrapped a Google IME call in
runBlocking with a one-second timeout, so a single direct message could
hold the tick thread for up to a second - twenty ticks - whenever the
words were not already cached. Conversion defaults to on for players once
the feature is enabled, so this was the ordinary path, not an edge case.
The conversion chain is suspending now, and the two commands dispatch
delivery to the plugin scope instead of running it inline. Nothing in that
chain touches world state: it sends chat components and plays client-side
sounds, both of which Paper already accepts off-thread, and which this
plugin already does from AsyncChatEvent.
AsyncChatEvent keeps a blocking bridge, renamed convertWithRomajiBlocking
so the choice is visible at the call site. That handler has to decide
whether to cancel the event and what body to set before it returns, and it
is already off the tick thread.
The commands take their scope as a constructor parameter so tests can
choose one. The new test uses StandardTestDispatcher to pin the property
that matters: execute() returns before delivery has run at all.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Every channel mutation built a full snapshot - copying the channel map,
every member list, and stringifying a UUID per active player - and then
queued a task that pretty-printed and rewrote all of channels.json. There
was no debounce, unlike the settings storage, so a fifty-player join wave
meant fifty snapshots and fifty whole-file writes. Joins and quits both go
through it via setPlayerChannel, and so does the self-healing branch of
getPlayerChannelContext, which sits on the channel-chat message path.
ChannelStorage now debounces like the other two storages, and takes the
snapshot as a supplier so it is built once when the write runs rather than
once per queued change. saveToStorage and saveToDisk had the same six-line
snapshot construction; that is now one private function.
The trade-off is the same one the settings storage already makes: a crash
within the debounce window loses the last few seconds of channel state.
Shutdown still writes synchronously.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
isMember went through getChannelMembers, which copies a channel's whole
member list defensively - to then run any{} over it and throw the copy
away. getPlayerChannels did that once per channel, so asking "which
channels is this player in" allocated an ArrayList per channel and scanned
every one. It runs on join, and on every /lc channel join when a
membership limit is configured.
ChannelManager now answers both directly against the live lists: isMember
scans in place, and channelIdsOf walks the membership map once. No copies,
and getPlayerChannels no longer enumerates all channels separately to
cross-reference them.
Not done: the reviewed suggestion was a playerId -> channelIds reverse
index for O(1) lookups. That means a second record of who is in what,
updated by hand at five mutation sites, where drift shows up as a wrong
membership answer rather than a crash. The allocation was the real cost
here, and removing it does not introduce a fact stored twice.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Spy notification ran per recipient what it could run once: both handlers
looked up general.spyMessage and rebuilt the hover component inside the
forEach, and the direct message path also allocated a Set inside the
filter predicate, once per spy per message. It formatted the spy copy of
the message before discovering there were no spies to send it to - and
spies are rare, so that was the normal case.
SpyPermissionManager.notifySpies now owns the whole shape. It takes the
body as a lambda so nothing is built for an empty audience, attaches the
hover once, and reads the roster directly rather than through
getDirectMessageSpyPlayers()'s defensive copy. It also puts "who must not
see this" in one place; the two handlers had drifted to expressing it by
name in one and by UUID in the other.
ConversionCache tracked no dirtiness, so the periodic task re-serialized
and rewrote the entire cache file on its fixed interval whether or not
anyone had chatted, while every single put scheduled another full rewrite
five seconds out. It now records that it changed and the periodic task is
the only writer, returning immediately when there is nothing to write. A
failed write marks the cache dirty again so the next tick retries.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
PlayerQuitEvent called saveToDisk(), which KAML-serializes every player
ever recorded and writes the file inline. That runs on the tick thread,
costs time proportional to the total stored players rather than the online
ones, and so grows for the life of the server. A logout wave stacks the
writes into a visible stall. The comment above the call already said
"async save"; the storage KDoc already said the synchronous path was for
shutdown.
Quitting changes no setting, so there is nothing new to persist - the
value of saving here is flushing what an earlier toggle left pending.
queueSave() does exactly that through the existing debounce, and
saveToDisk() is now documented as the shutdown path and called from
nowhere else.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
RomanjiConverter awaited the Google IME call for one word before starting
the next, though the words are independent. Callers convert under a single
one-second timeout covering the whole message, so an N-word message needed
N round trips inside a budget sized for roughly one: past the first word or
two the remaining calls were cancelled, and because cache.put is only
reached after a call returns, the discarded work was not even remembered.
The next identical message repeated it.
The words now run under one coroutineScope, so the message costs one round
trip rather than N.
The new test pins the concurrency itself rather than the timing: the fake
API client records how many calls are in flight at once, which fails
against the sequential version.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Every channel message wrote a fully interpolated INFO line, duplicating
what the dedicated async ChannelMessageLogger already persists. Every
cross-server message produced one INFO on the sending Paper server, two on
Velocity, and one on each receiving Paper server - so a single global chat
message cost up to four synchronous console and latest.log writes across
the network, on the message path, with JUL and Logback appenders being
synchronous.
These are now debug, and phrased so the string is not built unless debug
is on: the JUL sites take a supplier, the slf4j sites take a format plus
arguments.
Handshake, registration and delivery-failure lines stay at info - they
fire once per server or once per failed message, and they explain
something an operator needs to see.
Two relay tests asserted on the text of a log line. One of them was
verifying "0 servers" where the same test already verifies zero sends;
the other was really checking that the payload reaches the target, so it
now decodes the relayed bytes and compares them to the original message.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
Both HTTP consumers - Japanese conversion and the update check - default
to disabled, yet onEnable built an HttpClient(CIO) unconditionally,
starting a selector and dispatcher pool that a stock install never used
and nothing ever closed. It is now created on first use and closed on
disable, which also fixes the leak across /reload.
LanguageManager parsed and flattened every bundled language file, though
getMessage only ever reads the selected one and the English fallback. It
now loads exactly those two.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
DebouncedSaver keeps only the first callback of a burst, and
queueAsyncSave closed over the snapshot taken when it was called. So if
one player toggled a setting and a second toggled two seconds later, the
write that fired at five seconds persisted the first snapshot and dropped
the second player's change - it survived in memory until some later toggle
happened to trigger another write, and was lost on a crash.
Passing a supplier instead means the snapshot is taken when the write
runs, which is what "batched into a single save" was always meant to mean.
ConversionCache already had this shape by passing ::saveToDisk.
The staleness predates the refactor, but bc8010b claimed to have closed
this window; it only moved where the snapshot was built, not when.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
- MessageFormatter built the same prefix component in three functions.
- LanguageManager copied kaml's YamlNode into a private YamlValue tree
before flattening it, so the map case was written twice and the
list-of-maps case rendered a Kotlin data class toString into a player
facing string. It now folds YamlNode directly.
- StatusCommand inlined `if (enabled) "toggle.on" else "toggle.off"`,
which is the body of LanguageManager.getToggleText.
- The three chat formats each spelled out their own chain of
String.replace, with the valid placeholder names documented only in a
config.yml comment.
- ChannelContext carried a channelId that both construction sites filled
with channel.id; it is now derived, so the two cannot disagree.
- ChannelInfo and ChannelStatus each declared MAX_MEMBERS_DISPLAY = 10 and
built the same truncated member line, differing only in indent. A
divergence between the two constants would have been invisible.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
ServiceInitializer held ten nullable fields alongside the ServiceContainer
it returns. Six of them were written and never read; the remaining four
made shutdown() ambiguous, reading conversionCache and channelMessageLogger
from its own fields but everything else from the container it was handed.
LunaticChat then mirrored seven more into public vars, one of which
(channelMessageHandler) nothing read at all.
Now the container is the single place a service lives: the initializer
builds and returns, shutdown and the periodic task read from what they are
given, and the plugin's public properties delegate rather than copy. A new
service is one field instead of three, and no copy can go stale.
PlayerSettingsManager had the same shape at a smaller scale: three UUID
maps plus a PlayerSettingsData kept in sync by hand, where the data object
was a pure derivation rebuilt - three full map copies - on every toggle. It
now keeps one map and derives the snapshot at save time, which also closes
the window where queueAsyncSave captured state that changes before the
debounce fires.
Co-Authored-By: Claude <noreply@anthropic.com>
|
