Skip to content

test(runtime): a group join and leave rewrite no content rows - #8664

Merged
Thamirawaran merged 13 commits into
jaseci-labs:mainfrom
MusabMahmoodh:test/group-membership-invariant
Aug 31, 2026
Merged

test(runtime): a group join and leave rewrite no content rows#8664
Thamirawaran merged 13 commits into
jaseci-labs:mainfrom
MusabMahmoodh:test/group-membership-invariant

Conversation

@MusabMahmoodh

Copy link
Copy Markdown
Contributor

Summary

Closes #8460, as its own section 6.1 predicted: the group key turned out not
to be needed, because allow_group plus edge-based membership landed in #8598
(merged yesterday) with the pushdown. What #8460 asked for and did not get is
the proof and the app-facing documentation, which is what this PR adds.

I checked the mechanism against the issue's complaint before writing anything:

  • The grant is keyed by group, not by root, so sharing with an audience is one
    entry per object rather than N.
  • Membership is an edge resolved at check time (_groups_of), so a join or a
    leave writes no content row at all - the O(members x anchors) cost the issue
    measured (100 grant calls per join, 100 per leave, every row's props
    changed) is gone.
  • The remaining caveat the issue raises - access still living inside the
    props JSONB - no longer has the consequence it names, because membership no
    longer touches props. Moving access to its own column stays a legitimate
    storage follow-up but is not needed for I2; I did not fold it in here.

So the honest close is evidence plus docs, not a new mechanism.

What this adds

  1. The regression test the issue specifies, on real serialized rows:
    tests/runtimelib/test_group_membership_cost.jac seeds 8 group-shared
    items, fingerprints every content row (sha256 of the serialized props, which
    is what a store write sees), then joins a member and leaves again.
  2. allow_group documented in jac-sv-multi-user, next to the per-root
    allow_root section it replaces for audiences. It was only mentioned in
    jac-sv-persistence (a performance aside), so an app author reading the
    multi-user skill - the one that teaches sharing - would not find it and
    would reach for allow_root per member, which is exactly how the issue's
    O(N x M) pattern gets written.

Result on the rig: 1 passed.

Could this have been less code?

It is test + docs only; no runtime change. The alternative was implementing
the issue's option 2 (a Permission.groups map keyed by group id) - which
already exists as of #8598, so writing it again would have been duplicate
mechanism. Verified before starting rather than after.

Deleted

  • Nothing. Deliberately not deleted: the per-root allow_root path, which
    is still correct for sharing with one named user; the docs now say which to
    reach for. Nothing about the previous behavior was asserted anywhere, so no
    test became stale.

What else could this break

  • Test-and-docs only: no runtime, config, or storage surface is touched, so
    monolith vs microservice, kind vs EKS, embedded vs external DB and replica
    count are all unaffected.
  • The new test drives a real store in a temp directory through the same
    ExecutionContext helpers test_acl_pushdown.jac uses, so it exercises the
    real serializer and the real group resolution rather than asserting on
    internals.
  • One thing reviewers should sanity-check: the fingerprint is taken over
    Serializer.serialize(anchor, include_type=True), which is what
    _serialize writes into props. If that ever stops being the write path,
    this test would silently stop measuring the right thing.

MusabMahmoodh and others added 11 commits August 24, 2026 16:42
Asserts the membership-cost invariant on real serialized rows: every
content row is byte-identical across both a join and a leave while the
member's visibility flips, which is what distinguishes a group grant
plus a membership edge from per-root grants proportional to the group's
data. Documents allow_group in the multi-user skill, where the per-root
grant it replaces is taught.

Closes jaseci-labs#8460
Bare top-level statements aren't valid at module scope, the connect
operator was doubled (++>:Type():++> instead of +>:Type:+>), and doc/
user_root were never declared - none of it had been run through jac
check. Wrapped in functions with a self-contained Doc type, fixed the
operator, and swapped user_root for root (the joining principal's own
root, matching every other cross-user example in this file).
jaseci-labs#8682 dissolved jaclang/jac0core and jaclang/runtimelib. This test named
four modules by their old paths, so it merged clean and would have failed
at import: archetype and runtime moved to jaclang/runtime, context moved
to jaclang/runtime, and serializer moved to jaclang/data.
No source change. `passes-native`/`test-runtime` failed on a fault that
cannot originate in this branch (see the PR comment); this re-triggers the
lane so the real signal is visible.
@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.

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 ce377ab into jaseci-labs:main Aug 31, 2026
29 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

3 participants