Skip to content

fix(scale): fail closed when the asserted user root cannot be bound - #8657

Merged
Thamirawaran merged 10 commits into
jaseci-labs:mainfrom
MusabMahmoodh:fix/native-server-root-bind-fail-closed
Sep 1, 2026
Merged

fix(scale): fail closed when the asserted user root cannot be bound#8657
Thamirawaran merged 10 commits into
jaseci-labs:mainfrom
MusabMahmoodh:fix/native-server-root-bind-fail-closed

Conversation

@MusabMahmoodh

Copy link
Copy Markdown
Contributor

Summary

Fixes #8348.

request_context_middleware caught ValueError from aset_user_root, logged
"using system root", and let the request proceed on the execution context's
existing root - the system root, which bypasses the owner check
(check_access_level) and disables the traversal ACL gate (_acl_gated).
Today a redundant second bind in _run_function_with_occ happens to kill such
requests with a 500 traceback, so the tenant boundary holds by accident; any
request path without that second bind inherits a live fail-open. Because
MissingAnchorError extends ValueError, the swallow covered both a malformed
root id and a well-formed id that resolves to no anchor.

The middleware now fails closed: an asserted caller identity that cannot be
bound rejects the request with a deliberate 401 (UNAUTHORIZED envelope), and
the exception detail goes to the server log for the operator. Requests that
assert no identity (guest, :pub) are unchanged. The rejection is returned
inside the existing try/finally so context, auth, and tracing cleanup still run.

Test (real subprocess, real SQL, no mocks)

New scale/tests/server/test_root_bind_fail_closed.jac: real
jac run --serve subprocess; the fault is injected exactly the way the issue
measured it - real SQL against the server's identity store
(UPDATE identity_users SET doc = doc || ...) through a fixture endpoint.
Covers both variants (well-formed-but-unresolvable UUID, malformed id), with a
positive control before the fault and a fresh-user control after.
Fails on pre-fix code with unresolvable root must be rejected with 401, got 500; passes with the fix.

Could this have been less code?

  • Reused error_response in runtime/context/errors.jac (added a 4-line
    unauthorized helper beside service_unavailable / gateway_timeout).
  • Considered rejecting inside aset_user_root itself: rejected - it is a
    runtimelib primitive whose callers legitimately handle failure differently
    (the OCC path heals via reset_root); the middleware is where "asserted
    identity" is known.
  • Considered deleting the try entirely and letting the exception propagate:
    rejected - that reproduces today's observable bug (500 + traceback) rather
    than a deliberate 401.

Deleted

  • The "using system root" warning-and-continue branch - the fail-open recovery
    itself. Nothing else became dead: the downstream bind in
    _run_function_with_occ stays, as first-line resolution for paths that do
    not traverse this middleware.

What else could this break

Grepped aset_user_root, user_root_id, unauthorized, UNAUTHORIZED call
sites:

  • Clients that relied on the 500: no caller can sensibly depend on a
    traceback; they now get a structured 401. The issue names this as the
    expected behavior.
  • Healing paths: reset_root healing in _abegin_user_request_context
    runs only under JAC_SV_SIBLING and binds through its own path, not this
    middleware branch; unaffected (the middleware only rejects when the id it
    was handed cannot bind).
  • Guest and liveness paths: user_id unset skips the bind entirely;
    /healthz, /healthz/live short-circuit before this code. Unchanged.
  • Monolith vs microservice: the middleware runs in every native-served
    pod and in jac run --serve; behavior is identical. kind vs EKS, embedded
    vs external DB: not touched (the store only matters as the source of the
    corrupt id). Multi-replica: per-pod middleware, same rejection on each.
  • SSE/generator paths (A generator served as SSE executes against a different root: streamed handlers read an empty graph and their writes are lost #8136): they no longer start on the system root
    when the bind fails - previously the masking crash was the only thing
    standing between them and fail-open.

MusabMahmoodh and others added 7 commits August 24, 2026 15:48
request_context_middleware caught ValueError from aset_user_root, logged
a warning, and let the request proceed on the execution context's
existing root, which is the system root. The system root bypasses the
owner check and disables the traversal ACL gate, so any request path
without a redundant downstream bind was fail-open at the tenant
boundary. MissingAnchorError extends ValueError, so the swallow also
covered a well-formed root id that resolves to no anchor, and the
observable behavior was a 500 with a traceback from the second bind.

An asserted caller identity that cannot be resolved now rejects the
request with a 401 at the middleware; the exception detail stays in the
server log for the operator. Requests that assert no identity are
unchanged.

Fixes jaseci-labs#8348
# Conflicts:
#	jac/jaclang/scale/server/native_server.jac
…8454 did not carry

The middleware change this PR opened with landed on main as jaseci-labs#8454 (merged
2026-08-28), including `identity_unresolvable` in errors.jac and the
(ValueError, RuntimeError) fail-closed branch in native_server.jac. Both
production edits are dropped as duplicates.

What is left is the tier jaseci-labs#8454 did not have: a real server subprocess, a
real Postgres store, a SQL fault injected into identity_users, and the 401
asserted over HTTP with a positive control and an unaffected-user control.
jaseci-labs#8454's test_middleware_root_bind_refused.jac covers the same decision one
tier down, through make_client.
@MusabMahmoodh

Copy link
Copy Markdown
Contributor Author

CI note: the red lanes on this PR are not from this branch. Recording the
measurement here so a reviewer does not bounce it, and so it is not
re-diagnosed from scratch.

0.37.0 merged this morning (#8792). Two independent problems arrived with it,
both confirmed on PRs of mine that cannot possibly cause them:

1. passes-native is flaky. My #8654 is a docs-only PR - one markdown
file - and it failed passes-native with a compiler re-entrancy error:

CompilerSourceError: compiling jaclang/compiler/symbol_utils.jac re-entered it: a pass imported 'jaclang.compiler.symbol_utils' while Python was still executing that module

A markdown file cannot cause that. Both #8654 and #8664 went
failure -> success on an empty commit with no source change.

2. Whole-tree jac check fails on one random file per run. jac-check
runs jac check unscoped on PRs by design - ci.yml says so explicitly: "a
type error is a property of the whole program, so a scoped run answers a
different question than the push sweep."
Across four of my PRs it reported
912 passed, 1 failed every time, on a different file each time, none of
them touched by the PR:

PR file it failed on error shape
#8657 scale/identity/impl/user_manager.impl.jac Cannot return <Constants.SUPER_ROOT_UUID>, expected str | NoneType
#8660 cli/commands/impl/execution.impl.jac ExecutionEngine | NoneType not narrowed
#8662 runtime/na_stdlib/urllib/request.jac Cannot assign <Unknown> to bytes
#8664 lsp/server/impl/engine.impl.jac assorted <Unknown> / type[T]

Same 913 files, same "912 passed, 1 failed", different victim each run, and
every error is the same shape: a type that should be known resolving as
<Unknown> or as an over-narrow literal. That is one non-deterministic
failure landing randomly, not four latent defects - and it is why I have
not "fixed" user_manager.impl.jac, which would have papered over a
checker bug with a str() cast.

Other people's PRs look green only because their runs predate 0.37 (newest is
Aug 30 21:21Z; 0.37.0 merged 02:28Z today). Every open PR should expect this
on its next rebase.

Happy to file the jac check non-determinism separately with the four-run
table if that is useful - flagging rather than filing, since the work pool and
stub catalog are new this release and may already be known.

MusabMahmoodh and others added 3 commits August 31, 2026 17:41
jac check's fork work pool assigns files to workers differently on every
run, and a worker serving a file with an incomplete stub prelude reports
stdlib types as Self/<Unknown>. One unrelated file fails per run. No
source change here.
@Thamirawaran
Thamirawaran merged commit 937f06c into jaseci-labs:main Sep 1, 2026
54 of 56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

native_server swallows a failed root binding and continues on the system root; only a redundant downstream bind prevents a fail-open

3 participants