server: make a missing length delimiter recoverable, not fatal (#29)

[matthew6s]

Fixes #29.

Decision (per the issue): a missing/invalid length delimiter is recoverable. A transient malformed frame shouldn't tear down the session — and the client already treats delimiter_missing_message (DelimiterMissingBufferResend) as a resend trigger, so making the server send it is the smaller, more consistent change (it mirrors the existing empty/corrupt resend paths).

Changes:

• ServerSettings gains delimiter_missing_message (matching the client's value), so client and server now agree on the full protocol message set.
• _receive_buffer_with_ack no longer raises BluetoothServerError('Invalid length prefix') on a frame with no : (or a non-numeric prefix); it sends the resend request and continues.
• Added test_server_requests_resend_on_missing_delimiter driving the round trip (malformed frame → resend sent → valid frame → ack + payload).

All server tests pass locally.

[sraodev]

Reviewed the diff and ran the real BluetoothServer/BluetoothClient against each other over a fragmenting in-memory stream to check the liveness question. Detailed findings are inline.

The decision is the right one, and the change fixes a silent data-corruption bug that the issue didn't even ask about: on master, b"-1:data" parsed to data_size = -1, passed the len(remainder) < data_size check, and the server acked while handing a truncated b"dat" to the deserializer. isdigit() closes that. Verified end to end.

One blocking issue (inline on server.py): the continue removes the only condition that terminated a desynchronized stream, so an ordinary MTU-fragmented frame now hangs instead of erroring.

Two findings that can't be anchored to the diff:

• sdk/python/tests/ never runs in CI. .github/workflows/ci.yml runs scripts/smoke.py and examples/chat only; no job invokes pytest against the SDK tests. There's also no conftest.py or requirements file, so collection fails without PyBluez (bluetooth_service/client_socket.py:9 imports bluetooth at module scope). The new test ships with no regression protection. Corroborating symptom: tests/test_client.py:73 asserts b"14:payload-bytes" against a 13-byte payload and fails on master too — pre-existing, and only invisible because nothing runs these tests.
• The client branch this PR makes reachable is still untested. tests/test_client.py:79 covers only EmptyBufferResend. Issue #29's fourth acceptance criterion is that both peers agree on the message set; the server half is done, the client half is unverified.

Also: issue #29's first acceptance criterion asks for the decision to be recorded on the issue. It currently has no comments — the rationale lives only in this PR description.

[sraodev]

The server doesn't drain the in-flight frame before looping, so it emits one control message per leftover chunk instead of one per client frame. The control channel is itself unframed, so the client's single recv coalesces them and the exact-match set test at client.py:61 fails.

Observed directly in the fragmentation run above:

BluetoothServerError("Unexpected acknowledgement:
  b'CorruptedBufferResendDelimiterMissingBufferResend...")

The fault has moved from the server (clear message, at the point of failure) to the client (unrecognizable concatenation), while the server carries on believing the session is healthy. Distinct from the cap — worth resyncing after a resend request and emitting at most one control message per client frame.

[sraodev]

Leaving this open as a follow-up rather than a blocker — the resend budget bounds it now, so it fails loudly instead of hanging. Still real: the server does not drain the in-flight frame, so one oversized client frame produces several control messages, and the unframed control channel lets the client coalesce them into Unexpected acknowledgement: b"CorruptedBufferResendDelimiterMissing...". The proper fix is accumulating across recv until data_size bytes are present, which is a framing change, not a patch to this PR.

[sraodev]

Correct fix for the issue as filed. Worth noting the underlying hazard this PR is evidence of: the four protocol strings are now duplicated between ServerSettings here and ClientSettings at client_config.py:23-26, with nothing asserting the two agree.

That's not hypothetical — it's the cause of issue #29 itself. client_config.py:25 and the client.py:64 branch have existed since d2319c6 while no server ever sent the message, so the two halves of one wire protocol drifted for an entire release and nothing caught it. If either literal drifts again, every resend request silently becomes BluetoothServerError(f"Unexpected acknowledgement: ...") at client.py:72.

These are protocol constants, not per-side configuration. A shared definition both dataclasses reference would make the class of bug impossible to ship. Fine as a follow-up, but the duplication is now 3 strings deep.

[sraodev]

Follow-up, not a blocker for this PR. Tracking the shared-constant refactor separately.

[matthew6s]

Addressed the review in bd86777.

• Added max_resend_attempts (default 3) and a shared bounded resend path. A fragmented/desynchronized stream now raises BluetoothServerError after three requests instead of looping forever and emitting unbounded control messages.
• Kept the wire-protocol behavior scoped to the existing resend design. Accumulating arbitrary recv() chunks is not safe without a framing change: a short read cannot distinguish MTU fragmentation from the intentionally corrupt/short-frame case, and naively concatenating the next resend would acknowledge mixed payload bytes. The bounded path is the minimum fix suggested in the review and restores loud failure for unrecoverable desynchronization.
• Parametrized server coverage for no delimiter, empty, alphabetic, and negative prefixes. The assertions now pin the exact resend/ack sequence and persistence side effects.
• Added a regression test for the fragmented/desynchronized case and bounded failure.
• Added client coverage for DelimiterMissingBufferResend retransmission.
• Corrected the pre-existing 13:payload-bytes expectation so the Python tests can pass.
• Recorded the recoverable-with-a-bound decision on issue Python SDK: DelimiterMissingBufferResend is handled but never sent #29.

Verification: all 14 tests under sdk/python/tests pass with a scoped PyBluez stub; git diff --check passes. I left the broader pytest/CI wiring as follow-up scope, as suggested.

[sraodev]

Thanks for picking this one up, @matthew6s — and for writing up the reasoning rather than just the patch. Making the missing delimiter recoverable was the right call, and pointing at DelimiterMissingBufferResend already being a resend trigger on the client is exactly the argument that settles it.

Worth calling out something the change does that the issue never asked for: switching from int() to isdigit() closes a silent data-corruption bug. On master, b"-1:data" parsed to data_size = -1, sailed through the len(remainder) < data_size check, and the server ACKed while handing a truncated b"dat" to the deserializer — a short payload reported as a success. Your guard rejects it. I confirmed that end to end. That's a better fix than the issue was asking for.

The one thing that needs to change before this merges is in my inline comment on the continue: it removes the last condition that terminated a desynchronized stream. I ran the real BluetoothServer and BluetoothClient against each other over a fragmenting in-memory transport (MTU 668, stock buffer_size=1024), and an 800-byte payload — under the buffer size, so just ordinary fragmentation — behaves like this:

	outcome
master	BluetoothServerError('Invalid length prefix'), terminates in 2 recvs
this branch	server never terminates

I have a patch that fixes it and I'll get it onto this branch shortly — it adds ServerSettings.max_resend_attempts (default 5), counts consecutive resend requests per frame, and raises when the budget is spent, so the recoverable case stays recoverable and the unrecoverable one still fails loudly. Same shape as the existing discovery_retries/connect_retries convention in client_socket.py. With it, that 800-byte case ends in No usable frame after 5 resend requests; the stream is desynchronized instead of hanging.

It also parametrizes the test over b"abc:data", b":data" and b"-1:data" so the isdigit() arm is actually pinned — worth knowing that today, deleting not data_size_str.isdigit() from your guard leaves the whole suite green while the truncation bug comes straight back — and adds DelimiterMissingBufferResend to the client-side resend test so the precondition your comment relies on is verified.

Nothing here needs action from you unless you'd rather make these changes yourself, in which case say so and I'll leave it to you. The remaining inline comments (stream resync / control-message framing, and the duplicated protocol constants) are follow-up issues, not blockers for this PR.

[matthew6s]

Looks like our updates crossed by about a minute — I pushed bd86777 just before this comment landed. It implements the same bounded-retry shape, parametrized malformed-prefix coverage (including b"-1:data"), exact message/persistence assertions, and client-side DelimiterMissingBufferResend coverage. All PR checks are green now.

The only intentional difference from the patch you described is the default retry budget: I used 3 rather than 5. Happy to align it to 5 (and your preferred error wording) if you want that convention here; otherwise the branch should already cover the blocker, so you shouldn't need to duplicate the patch.