Skip to content

feat(dropfile): add configurable limits for drop file - #216

Merged
Dream-Master merged 18 commits into
GTNewHorizons:masterfrom
hinyb:feat/drop-file-config
Aug 30, 2026
Merged

feat(dropfile): add configurable limits for drop file#216
Dream-Master merged 18 commits into
GTNewHorizons:masterfrom
hinyb:feat/drop-file-config

Conversation

@hinyb

@hinyb hinyb commented Jul 29, 2026

Copy link
Copy Markdown

Summary

This PR makes the previously hardcoded 64kb upload size limit configurable, and fixes the packet too large error that occurred with larger files, enabling support for much larger files.

Checklist

  • I have tested this PR in DevEnv
  • I have tested this PR in Fullpack
  • This PR is in compliance with the GTNH AI Policy
  • This PR requires another PR in order to merge

@hinyb hinyb added Bug Fix Fixes a bug. Please link it in the PR if an issue exists for it. New Feature Add something new. Please explain in detail how it works. labels Jul 29, 2026
@Dream-Master

Copy link
Copy Markdown
Member

@hinyb can you resolve conflicts?

@hinyb

hinyb commented Jul 30, 2026

Copy link
Copy Markdown
Author

@hinyb can you resolve conflicts?

Sorry for the late reply, resolved.

@Eldrinn-Elantey Eldrinn-Elantey left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed the chunked drop file transfer. The chunking itself correctly solves the 32767 byte packet limit, sizes are validated on both sides and sessions are limited to one per player with a TTL, so unbounded session count is not an issue.

There is one blocker: decompression is unbounded, so raising the limit to 4 MB opens a zip bomb path to server OOM. Details inline, together with a few smaller correctness and compatibility notes.

One process note: the DropFile packet format changed incompatibly without a version or flag, so an old client against a new server silently breaks. #215 made clipboard batching opt-in for exactly that reason. If a synchronized client/server update is intended here, it is worth stating that in the PR description.

Comment thread src/main/scala/li/cil/oc/server/DropFileSession.scala Outdated
Comment thread src/main/scala/li/cil/oc/server/DropFileSession.scala Outdated
Comment thread src/main/scala/li/cil/oc/server/DropFileManager.scala
Comment thread src/main/scala/li/cil/oc/server/DropFileManager.scala Outdated
Comment thread src/main/scala/li/cil/oc/server/PacketHandler.scala Outdated
Comment thread src/main/scala/li/cil/oc/Settings.scala
Comment thread src/main/scala/li/cil/oc/client/PacketSender.scala Outdated
Comment thread src/main/scala/li/cil/oc/client/PacketSender.scala Outdated
Comment thread src/main/resources/assets/opencomputers/lang/en_US.lang Outdated
Comment thread src/main/scala/li/cil/oc/Localization.scala Outdated

@Eldrinn-Elantey Eldrinn-Elantey left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Second pass over the changes since 403bc8f. All the earlier points are addressed, thanks. The bounded inflate, the on demand buffer, readFully, the session cleanup on logout and the server side name check all look right in the current code, and the replies about the API signature and the hardcoded name limit are fair.

A few new things in the code that was added, one of them a real regression from the rename.

Comment thread src/main/scala/li/cil/oc/Localization.scala Outdated
Comment thread src/main/scala/li/cil/oc/server/DropFileManager.scala
Comment thread src/main/scala/li/cil/oc/server/DropFileManager.scala Outdated
Comment thread src/main/scala/li/cil/oc/server/DropFileSession.scala Outdated
Comment thread src/main/scala/li/cil/oc/server/DropFileManager.scala
Comment thread src/main/scala/li/cil/oc/server/PacketHandler.scala Outdated
Comment thread src/main/scala/li/cil/oc/server/DropFileManager.scala Outdated
@Eldrinn-Elantey

Copy link
Copy Markdown

One open question about maxDropFileCount. It now carries two meanings: the number of files allowed per drop on the client, and both the bucket size and the refill rate of the server side limiter. With the default of 16, dropping the full 16 files at once empties the bucket, so a second legitimate drop within the same second is rejected part way through and the player gets a chat message per rejected file.

Was that intended, or would a separate rate setting (or a bucket with some headroom over maxDropFileCount) fit better? Not blocking either way, I just want to make sure the interaction is deliberate.

@hinyb

hinyb commented Aug 30, 2026

Copy link
Copy Markdown
Author

One open question about maxDropFileCount. It now carries two meanings: the number of files allowed per drop on the client, and both the bucket size and the refill rate of the server side limiter. With the default of 16, dropping the full 16 files at once empties the bucket, so a second legitimate drop within the same second is rejected part way through and the player gets a chat message per rejected file.

Was that intended, or would a separate rate setting (or a bucket with some headroom over maxDropFileCount) fit better? Not blocking either way, I just want to make sure the interaction is deliberate.

Yeah, it's intentional.I think reusing maxDropFileCount for the rate limit is fine here, cause nobody manually drags and drops files multiple times within a second, so I didn't add a separate config. The 1s throttling is reasonable to me.

@Eldrinn-Elantey Eldrinn-Elantey left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@Dream-Master
Dream-Master merged commit c3a147a into GTNewHorizons:master Aug 30, 2026
1 check passed
@hinyb
hinyb deleted the feat/drop-file-config branch August 31, 2026 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Fix Fixes a bug. Please link it in the PR if an issue exists for it. New Feature Add something new. Please explain in detail how it works.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants