cromper (third time's the charm)

[ethteck]

This PR adds a new entirely separate service for handling the various processes involves in running the site: assembling, compiling, diffing, and running decompilers.

The backend still talks to this service directly, as well as the frontend server, but now the client can also talk directly to this server to do decompilation tasks, incremental compilations, etc.

There's several reasons we are separating these into a separate service:

• The django server can be freed of many of its responsibilities
• The new service is async (woo), so requests will no longer block each other
• The backend is now much leaner, allowing for possible future rewrites and optimizations without worrying about all this compiler junk

Todo:

• Make compiler, library, and platform endpoints singular
• Deserialize back into proper objects in Django
• Django tests pass
• Fixup frontend /platform endpoint to fetch platform data from cromper (or update api.ts to be able to call cromper?)
• Sort out compiler flags
• Update nginx config
• health checks from django <> cromper?

[Copilot]

Copilot review overview

🟡 Changes recommended

Assembly cache collisions, lost assembler diagnostics, and broken setup paths remain unresolved.

Review effort: Balanced
Findings: 4 · 7

Open (11)

• Assembly cache key omits platform and result identity · New
• Clean Compose setup lacks required compiler and library assets · New
• Python 3.10 support conflicts with typing.Self import · New
• Integration test script resolves Compose path outside repository · New
• Assembler errors return HTTP 500 instead of validation diagnostics · New
• Native tests omit localhost Cromper URL configuration · New
• Native frontend routes API requests to removed Django endpoints · New
• The library root is set only in the parent process, but compile work runs in a…
• The new tag is persisted and displayed, but no deploy path ever assigns CROMPER_TAG from…
• After a context-related M2CError, this retries the exact same call with the same context before…
• Compiler metadata is cached for the lifetime of each Django worker, and refresh_cache has no…

Resolved since last review (5)

• The PATCH has already persisted the save before this new language request runs. If cromper is…
• Valid JSON is not necessarily an object. Sending [] or null passes this parser, after which…
• These absolute defaults break the documented native workflow (`cd cromper && uv run python -m…
• main is synchronous, so it executes (and blocks in ioloop.start()) before asyncio.run is…
• CROMPER_NUM_THREADS is parsed into config.num_threads, but this executor uses num_processes,…

[mkst]

cromper_client.py review

Summary

The overall direction looks good.

Moving compilation, assembly, decompilation, diffing, and associated compiler/platform metadata out of Django and into a separate cromper service is a sensible boundary. The current client is already fairly small and readable, and Django is moving toward consuming cromper as a service rather than knowing how those operations are implemented.

The main improvements I would make are around treating cromper as a genuine remote-service boundary rather than effectively a Python function that happens to speak HTTP.

The most important areas are:

1. Distinguishing transport failures from ordinary HTTP/application errors.
2. Simplifying the current service-recovery/cache state.
3. Strictly validating successful cromper responses.
4. Returning typed domain results rather than protocol-shaped dictionaries.

None of these require a major redesign.

---

1. _make_request() conflates request rejection with service unavailability

This is the most important issue.

Currently _make_request() calls:

response.raise_for_status()

inside a try block whose RequestException handler does:

self._service_available = False
raise CromperUnavailableError(...)

This means that a healthy cromper returning an ordinary HTTP error such as:

POST /compile
400 Bad Request

is treated as though cromper is unavailable.

That also affects the cache/recovery behaviour: the next metadata lookup can probe /healthz, decide cromper has "recovered", and invalidate caches even though the service never actually went away.

Suggested approach

Distinguish at least:

• connection failure -> CromperUnavailableError
• timeout -> CromperTimeoutError
• HTTP 4xx -> request/application/protocol error
• HTTP 5xx -> cromper-side error, but not necessarily "unreachable"
• malformed JSON -> protocol error

For example:

try:
    response = self.session.request(
        method,
        url,
        timeout=self.timeout,
        **kwargs,
    )
except requests.Timeout as e:
    self._service_available = False
    raise CromperTimeoutError(f"cromper timeout: {e}") from e
except requests.ConnectionError as e:
    self._service_available = False
    raise CromperUnavailableError(f"cromper unavailable: {e}") from e

if response.status_code >= 400:
    raise CromperError(...)

The key point is that an HTTP response from cromper proves that cromper is reachable. An HTTP 400/404/500 should therefore not automatically mark the service as unavailable.

---

2. The response contract is currently too permissive

Several successful responses can be malformed without the client noticing.

For example, compile_code() currently does:

elf_object_b64 = response.get("elf_object", "")
elf_object = base64.b64decode(elf_object_b64)

So this response:

{
  "success": true
}

quietly becomes:

elf_object == b""

rather than being identified as an invalid cromper response.

Similar cases exist elsewhere:

• assembly accepts missing hash
• assembly accepts missing arch
• assembly accepts missing elf_object
• diff accepts missing result
• decompile turns missing decompiled_code into an empty string

That risks surfacing a cromper bug several layers later as an ELF parsing error, missing-data error, or some other less obvious failure.

Suggested approach

Treat required fields as required:

try:
    elf_object_b64 = response["elf_object"]
except KeyError as e:
    raise CromperError(
        "Invalid /compile response: missing elf_object"
    ) from e

Base64 should probably also be strictly validated:

import binascii

try:
    elf_object = base64.b64decode(
        elf_object_b64,
        validate=True,
    )
except (ValueError, binascii.Error) as e:
    raise CromperError(
        "Invalid /compile response: malformed elf_object"
    ) from e

This does not need a heavyweight validation framework. Small endpoint-specific parsing helpers would be enough.

---

3. CompilationResult and DiffResult exist but are not used

The file defines:

@dataclass
class CompilationResult:
    elf_object: bytes
    errors: str


@dataclass
class DiffResult:
    result: dict[str, Any] | None
    errors: str

but the interface still exposes:

def compile_code(...) -> dict[str, Any]:

and the implementation returns dictionaries.

This looks like a half-finished refactor and is worth completing.

Suggested approach

Expose typed results from the client:

def compile_code(...) -> CompilationResult:
    ...

and return:

return CompilationResult(
    elf_object=elf_object,
    errors=response.get("errors", ""),
)

Likewise for DiffResult, and probably an AssemblyResult.

This creates a useful boundary:

Django code
    |
    | CompilationResult / DiffResult / AssemblyResult
    v
CromperClient
    |
    | HTTP / JSON / base64 / status codes
    v
cromper

Django should not need to care that an ELF object happened to cross the service boundary as base64.

---

4. _service_available is currently an awkward state machine

The current behaviour is approximately:

1. A request fails.
2. _service_available becomes False.
3. Only get_compilers() / get_platforms() call _probe_recovery().
4. _probe_recovery() calls /healthz.
5. Recovery invalidates metadata caches.

There is an edge case here.

Suppose:

1. /compile times out.
2. _service_available = False.
3. cromper recovers immediately.
4. another /compile succeeds.
5. _service_available remains False.
6. later, get_compilers() is called.
7. the client unnecessarily probes /healthz and invalidates its caches.

At minimum, a successful request should restore the availability state.

However, the state can probably be simplified further.

Suggested approach

The actual requirement appears to be:

If there was a genuine transport failure and we later successfully communicate with cromper again, metadata may have changed and should be refreshed.

That could be represented more directly:

_had_transport_failure = False

Then:

except (requests.Timeout, requests.ConnectionError):
    self._had_transport_failure = True

and after the next successful request:

if self._had_transport_failure:
    self._invalidate_caches()
    self._had_transport_failure = False

This would remove the need for the separate health probe entirely.

That feels simpler and models the behaviour actually required.

---

5. /healthz currently has to return JSON

_probe_recovery() calls:

self._make_request("GET", "/healthz")

but _make_request() always does:

return response.json()

So /healthz is implicitly required to return valid JSON.

That may already be intentional, in which case it is fine, but it is worth making explicit as part of the cromper API contract.

Otherwise either:

• allow _make_request() to support responses without JSON, or
• remove the explicit health probe as part of simplifying recovery behaviour.

---

6. Cromper being authoritative for metadata is the right direction

One architectural aspect that looks particularly good is that compiler/platform metadata is being sourced from cromper rather than duplicated in Django.

For example, Django reconstructs Compiler objects from the /compiler response, including their platform and language details.

That is a good service boundary.

The long-term direction should ideally be:

Django knows that a compiler exists, what capabilities it exposes to users, and how to ask cromper to perform work.

Django should not need to know:

• compiler installation paths
• process execution details
• compiler-specific runtime quirks
• sandbox mechanics
• diff-tool execution details
• decompiler execution details

The current implementation is already heading in that direction.

I would therefore avoid adding significantly more abstraction around the HTTP client itself. The current client layer is approximately the right amount of architecture.

---

7. get_platform_by_id() can be simpler

Currently the code iterates the platforms dictionary:

platforms = self.get_platforms()
for id, platform in platforms.items():
    if id == platform_id:
        return platform
raise ValueError(f"Unknown platform: {platform_id}")

This can simply be:

try:
    return self.get_platforms()[platform_id]
except KeyError:
    raise ValueError(
        f"Unknown platform: {platform_id}"
    ) from None

This is minor, but clearer.

---

8. Platform metadata could use the same consistency check as compiler metadata

Compiler loading carefully validates that the ID inside the response matches the dictionary key:

response_id = compiler_data.get("id", compiler_id)
if response_id != compiler_id:
    raise ValueError(...)

Platform loading currently does:

self._platforms_cache = {
    k: Platform(**v)
    for (k, v) in response.items()
}

If platform responses also contain an id, it would be useful to apply the same consistency check.

The client is a good place to fail loudly if the remote service emits internally inconsistent metadata.

---

9. Cached dictionaries are mutable by callers

Methods return the cached dictionaries directly:

return self._compilers_cache

and:

return self._platforms_cache

So callers can accidentally mutate global cached state.

This is probably low risk if existing calling code treats them as immutable, so it is not necessarily worth changing immediately.

Possible options include:

• expose Mapping[str, Compiler] / Mapping[str, Platform]
• return a shallow copy
• leave it as-is but treat mutation as unsupported

This is mainly about making the intended contract explicit.

---

10. Preserve exception chaining consistently

Compiler metadata validation already does this well:

raise CromperError(...) from e

The transport handlers currently do not.

Prefer:

raise CromperTimeoutError(...) from e

and:

raise CromperUnavailableError(...) from e

This keeps the original requests exception available for debugging while still presenting a cromper-specific exception to calling code.

---

11. Consider separate connect and read timeouts

The current client uses a single timeout value:

timeout=self.timeout

For a local/internal cromper service, ten seconds is probably very generous for establishing a connection, while compilation/diff/decompilation work might legitimately need longer.

requests supports:

timeout=(connect_timeout, read_timeout)

For example:

timeout=(1, 30)

The exact values are workload-dependent.

The important point is that cromper itself should remain responsible for enforcing the actual execution limits of compilers/decompilers. The Django-side timeout is primarily about how long Django is prepared to wait for the service response.

---

12. Consider the global requests.Session()

The global client contains a single:

requests.Session()

and the client itself is also a process-global singleton.

There is no obvious problem here because the session does not appear to be mutating cookies or per-request headers, and keeping a session gives useful HTTP connection reuse.

However, because Django deployments are commonly threaded, it is worth being conscious that this is shared mutable state.

I would not change it automatically, but it is worth deciding deliberately whether the intended model is:

• one shared session
• thread-local sessions
• no persistent session

The current implementation is probably fine as long as the session remains effectively configuration-only.

---

Recommended changes before merge

I would keep this PR focused and make four changes:

1. Separate transport failures from HTTP/application failures.
2. Make successful communication clear/reset the recovery state, or simplify the recovery mechanism entirely.
3. Strictly validate required response fields from cromper.
4. Use CompilationResult / DiffResult / an equivalent AssemblyResult instead of returning protocol dictionaries.

Everything else can reasonably be treated as cleanup or follow-up work.

---

Architectural rule worth preserving

A useful rule for this boundary going forward is:

Everything HTTP/JSON/base64/cromper-protocol-specific should end inside CromperClient; everything above it should deal in decomp.me domain objects.

The current code is already most of the way there.

The remaining dict[str, Any] return values and permissive response parsing are the main things preventing this from being a particularly clean boundary.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved compile/diff failures, assembly-key collisions, validation regressions, and service setup/deployment issues block approval.

Review effort: Balanced
Findings: 4 · 5

Open (9)

• Compile action reconstructs already-validated Library objects · New
• Successful diffs emit null errors instead of an empty string · New
• Clean Compose setup lacks required compiler and library assets
• Assembly cache key omits platform and result identity
• Mock returns identical hashes for distinct targets · New
• Native frontend routes API requests to removed Django endpoints
• Native tests omit localhost Cromper URL configuration
• The library root is set only in the parent process, but compile work runs in a…
• The new tag is persisted and displayed, but no deploy path ever assigns CROMPER_TAG from…

Resolved since last review (5)

• Integration test script resolves Compose path outside repository
• Python 3.10 support conflicts with typing.Self import
• Assembler errors return HTTP 500 instead of validation diagnostics
• After a context-related M2CError, this retries the exact same call with the same context before…
• Compiler metadata is cached for the lifetime of each Django worker, and refresh_cache has no…