Skip to content

Update and fetch - #4

Merged
orweis merged 50 commits into
masterfrom
update_and_fetch
Mar 8, 2021
Merged

orweis merged 50 commits into
masterfrom
update_and_fetch

Conversation

@orweis

@orweis orweis commented Mar 3, 2021

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread opal/client/data/fetcher.py Outdated
Dict[str, Any]: urls mapped to their resulting fetched data
"""
# return value - /fetch-results mapped by url
results_bu_url = {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: results_by_url

Comment thread opal/client/data/updater.py Outdated
urls = {entry.url: entry.config for entry in entries}
url_to_entry = {entry.url: entry for entry in entries}
# get the data for the update
logger.info("Fetching policy data", urls=urls.keys())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

urls might be None

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's fine fetch_policy_data has a default for None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@orweis you did not understand, if urls is none and you call urls.keys(), an exception will be thrown in the logger.info line.

@@ -35,7 +35,7 @@ class OpaClient:
"""
POLICY_NAME = "rbac"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn't OpaClient inherit from BasePolicyStoreClient?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

@asafc asafc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

overall looks good, some minor issues i found (see comments)

Comment thread opal/common/aio_requests.py Outdated
@property
def session(self):
""" An instance of aiohttp.ClientSession """
# fix DeprecationWarning: client.loop property is deprecated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this comment still relevant?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. it explains what the code does...

@orweis

orweis commented Mar 8, 2021 via email

Copy link
Copy Markdown
Contributor Author

@orweis
orweis merged commit f4c6227 into master Mar 8, 2021
@asafc
asafc deleted the update_and_fetch branch March 14, 2021 07:25
roekatz pushed a commit that referenced this pull request Jun 9, 2023
…eue-length

Track worker queue size in OPAL worker
dshoen619 added a commit that referenced this pull request Jul 1, 2026
Zeev's gate-coverage review found only 2 of 5 flagship tests (churn #1,
offline #4) were genuine fail-now / pass-after gates. Lift the other three:

- #5 broadcaster: run 2 workers (OPAL_TEST_WORKERS) so the Postgres backbone
  is actually fanned out cross-worker (references/debug-pubsub.md §3-4), and
  assert the gunicorn worker PIDs are unchanged across a transient bounce --
  the in-place-reconnect signal that distinguishes #915 (PER-15065) from a
  plain worker respawn. Prove recovery via a servable post-bounce scope, not
  /internal cache counts (per-process, non-deterministic on 2 workers).
- #3 boot: key completion on "all scopes served" (GET /scopes/{id}/policy ==
  200) instead of repo_locks (set at fetch start, so it undercounts the final
  clone); document the PR4 tight-BOOT_TARGET_SECONDS carry-forward.
- #2 repeat-sync: rename to test_repeat_sync_rss_stays_bounded and drop the
  tautological len(repos) assertion; RSS is the sole (load-bearing) gate.

Adds worker_pids() (/proc-based, matched host-side so the scan can't count its
own sh -c wrapper) and the opal_multiworker fixture (recreate to 2 workers,
restore to 1 on teardown).

Validated live (--boot-scopes=50): #2/#3 pass, #5 passes (worker PIDs held
across the bounce, post-bounce scope served), #1/#4 fail for the right reason
(PR2/PR3 not landed).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dshoen619 added a commit that referenced this pull request Jul 1, 2026
Addresses the HIGH and all MEDIUM findings from the opal-development /
python-pro / backend-architect microreview:

- H1 (#5 vacuous pass): the broadcaster gate now positively verifies a
  disconnect+reconnect actually happened, not just that no respawn occurred.
  New helpers.broadcaster_connect_count() counts the reconnecting broadcaster's
  "listener connected to channel" log line; the test asserts it increased across
  the bounce (paired with worker PIDs unchanged = in-place, not respawn).
- M1 (#4): move the 40 executor-saturating PUTs inside the try/finally so a PUT
  failure still runs hard_reset() instead of leaking hung clone threads into the
  session stack.
- M2 (#1/#2): _wait_until now treats a transient requests error from opal.stats()
  as "not yet" and retries, instead of ERRORing the test.
- M3 (#3): measure a deterministic pure-cold boot (--force-recreate wipes the
  ephemeral FS -> preload cold-clones all N from Redis) instead of a
  nondeterministic warm/cold mix, so PR4's tight BOOT_TARGET_SECONDS can gate.
- M4: verify the single-worker invariant -- opal_multiworker teardown asserts the
  stack is back to 1 worker, and the opal fixture asserts single-worker at setup,
  so a botched restore fails loudly instead of silently breaking cache gates.
- M5 (#4): correct the reserved-probe comment (serving shares the fetch executor,
  so a shared repo would be starved on serve too; the probe additionally
  exercises the clone).
- M6: gitea-admin retries on "database is locked" (CLI mutating live SQLite);
  rewritten as a `|` literal block so the create call stays on one line.

Validated live (--boot-scopes=20): #2/#3 pass, #5 passes (reconnect count
increased across the bounce + PIDs unchanged + scope served), #1/#4 fail for the
right reason.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Zivxx added a commit that referenced this pull request Jul 12, 2026
* feat(opal-server): gated /internal git-fetcher cache stats endpoint

Add an off-by-default diagnostics endpoint so tests can observe the
in-memory GitPolicyFetcher cache sizes (repo_locks/repos/repos_last_fetched)
and process RSS that the upcoming memory-leak fix eliminates.

- debug_stats.py: read-only git_fetcher_cache_stats() helper + a
  register_internal_stats_route() registrar that mounts
  GET /internal/git-fetcher-cache-stats only when enabled.
- config.py: new OPAL_DEBUG_INTERNAL_STATS flag, default False.
- server.py: register the route, gated by the flag, beside /healthcheck.

No production behavior change when the flag is off (the default). Also
ignore .claude/ so private planning artifacts are never committed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(git-leak): add OPAL git leak/resilience test bed

A self-contained docker-compose stack (opal-server x2 workers + Redis +
Postgres broadcaster + Gitea) plus a pytest harness that reproduces, as
tests that fail on master, the git-fetcher memory leak, the offline-repo
hang, the slow serial boot, and the broadcaster-disconnect gap. These
become the regression gates for the follow-up fixes.

- seed/: idempotent Gitea seeding sidecar (N policy repos) + Dockerfile.
- docker-compose.yml: 4-service stack, opal-server built from the repo's
  own docker/Dockerfile (server target), scopes on, Postgres broadcaster.
- helpers.py / conftest.py: HTTP + infra helpers and stack fixtures.
- test_leak.py / test_resilience.py / test_boot.py: the flagship tests.
- README.md: how to run and expected fail-on-master behavior.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(git-leak): add GiteaAdmin and make_repo_unreachable helpers

Complete the helpers.py surface promised in the plan's file-structure
table. Both are now functional and used, not dead code:

- make_repo_unreachable(name): returns a git URL on a routable-but-dead
  TEST-NET-1 host (RFC 5737). test_offline_repo now uses it instead of an
  inlined literal.
- GiteaAdmin: host-side Gitea admin client (list_repos / repo_exists /
  create_repo / delete_repo), exposed as the `gitea_admin` pytest fixture
  for tests that need to inspect or stage repos beyond the seed sidecar.

Gitea is published on host port 13000 (uncommon, to avoid the usual :3000
clash) so GiteaAdmin can reach it; opal_server and the seed sidecar still
use the internal http://gitea:3000. README updated with the helper and
port notes.

Verified live: GiteaAdmin lists the seeded repos and round-trips
create/exists/delete against Gitea over the published port.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(git-leak): correct postgres-bounce framing (passes on master)

Verified test_server_recovers_after_postgres_bounce against the stack: it
PASSES on master (~14-19s). On a broadcaster drop the affected worker
triggers a graceful shutdown, gunicorn respawns it, and the sibling worker
keeps serving HTTP, so the surface recovers within the window — recovery
happens via gunicorn's in-container worker supervision, not an external
supervisor and not an in-process reconnect.

Reframe #5 as a recovery guard (not a known-broken case) in the docstring
and README; the prior "FAILS on master / needs external supervisor" wording
was wrong. PER-15065's in-process reconnect would avoid the worker churn but
recovery already holds.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* style(git-leak): apply black/isort/docformatter (pre-commit)

Run the repo's pinned pre-commit formatters (black 23.1.0, isort 5.12.0,
docformatter 1.7.5) over the PR1 files to satisfy the pre-commit CI check.
Formatting only — no behavior change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test: scope root pytest collection to packages/ (exclude git-leak bed)

The CI `build` job runs `pytest` from the repo root with no path, which
recursed into app-tests/git-leak/ and ran the flagship tests — these are
designed to FAIL on master (they are the regression gates for PR2-PR5), so
they broke the build job.

Set `testpaths = packages` so the rootdir run collects only the unit tests
under packages/ (matching master's effective behavior, since app-tests/ had
no pytest files before). testpaths only applies when pytest is invoked from
the rootdir with no args, so `cd app-tests/git-leak && pytest` still collects
and runs the test bed. Verified both: root run -> packages only; subdir run
-> all 5 flagship tests.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(git-leak): address Copilot review feedback

- stats(): sample the /internal endpoint several times and merge per key
  with max(). The caches are per-process on a multi-worker server, so a
  single read can hit an empty (non-leader) worker — max-merge avoids both
  false negatives (missing a populated leader) and false positives (an
  `== 0` drain assertion passing only because it hit an empty worker).
- test_leak: assert the initial-load `_wait_until` succeeded before deleting
  / before taking baseline, so the tests can't pass vacuously when load
  never completed.
- refresh_all(): correct the misleading comment — a 404 is a no-op, there
  is no client-side fallback.
- conftest: skip the suite cleanly if docker is unavailable (defense in
  depth; it's already excluded from the default pytest run via testpaths).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(git-leak): isolate scopes per test and fix false repeat-sync gate

Two review findings on the regression gates:

- Cross-test contamination: clone paths are keyed by repo URL (source_id =
  sha256(url)+branch-shard), not scope_id, so scopes from different tests that
  point at the same seeded repo share one GitPolicyFetcher cache entry. With a
  session-scoped stack and no teardown, a leftover boot-*/stable-* scope kept
  those entries alive and would make test_churn's `repos == 0` drain assertion
  fail on fixed code. OpalServerClient now tracks created scopes and the opal
  fixture deletes them on teardown (best-effort drain wait, swallows errors so
  master — where delete never purges — doesn't fail the passing test).

- False gate: test_repeat_sync_does_not_grow re-syncs identical scopes, which a
  path-keyed cache can't grow even on master, so it could never be the leak gate
  it claimed. Reframed as an honest idempotency guard (passes on master) that
  points at test_churn_releases_caches as the real leak gate; README's
  "Expected on master" reclassifies it alongside the postgres-bounce guard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(git-leak): make the regression gates trustworthy (address PR review)

Addresses the CHANGES_REQUESTED review on PR #922. Root cause behind most
findings: a fresh scope's first sync takes the _clone() branch, which only
fills GitPolicyFetcher.repo_locks; repos/repos_last_fetched are filled on a
*second* sync. So the load gates on `repos` hung, and the 2-worker per-process
caches made `== 0` drain assertions unsafe.

Blocking:
- Single worker (UVICORN_NUM_WORKERS=1): deterministic per-process cache reads;
  removes the false `== 0` drain class.
- Load gate (CRITICAL + Zivxx HIGH): _load_scopes gates on repo_locks then
  refresh_all() to force the second sync, so repos/repos_last_fetched are
  actually populated before any drain/purge assertion.
- compose() surfaces captured stdout/stderr on failure.
- Seed completeness asserted in conftest; seed script isolates per-repo
  failures and exits non-zero with a count.

Secondary:
- test_repeat_sync asserts an RSS bound (count can't grow for any impl);
  churn asserts all three caches drain + a loose RSS backstop.
- blackhole socat sidecar replaces TEST-NET-1 (deterministic hang); offline
  test saturates the fetch executor with 40 hung clones and recovers via
  OpalServerClient.hard_reset() (stop -> redis FLUSHALL -> start).
- Per-test clean slate deletes all server scopes (fixes orphan-scope leak).
- Postgres-bounce proves broadcast recovery (PUT post-bounce, assert sync)
  and uses `up -d --wait`.
- Remove dead 404 branch in refresh_all; boot clock starts at restart;
  gitea-admin `|| true` -> "already exists"-only guard; README reworded.

opal-server:
- /internal stats route now takes the JWTAuthenticator dependency (protected
  when JWT on, no-op in the test bed); unit test asserts enforcement.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* style(git-leak): apply black/isort/docformatter (pre-commit)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(git-leak): tighten stat polling and pin test-bed images (PR review)

- snapshot a single /internal stats read per poll in the churn-drain and
  boot gates (consistent multi-key observation; fewer HTTP round-trips)
- document why a 200 from the healthy scope can't be a masked default
  bundle, and why the stats route is intentionally a sync def
- pin alpine/socat and the seed image's pip deps for reproducibility

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(git-leak): isolate offline-hang healthy probe to a never-cloned repo

Ziv (PR review, round 2) caught that the offline-resilience gate can
false-PASS in the full-suite run. The "healthy" scope pointed at
policy-repo-0000 (list_seeded_repos(1)[0]), but on-disk clones are keyed
by URL-hash and survive compose restart/stop/start (opal_server mounts no
volume at /opal; only `down -v` wipes them). test_boot/test_leak run first
(alphabetical) and already clone every seeded repo, so the healthy scope
hit the existing clone via _discover_repository, skipped _clone(), and
served 200 without ever touching the saturated fetch executor — the gate
that must FAIL on this branch (no PR3 timeout) passed.

Fix: seed a reserved repo (policy-repo-healthy-probe) outside the numeric
policy-repo-NNNN range that no boot/leak test enumerates, and point the
healthy probe at it. A never-cloned repo forces a genuine fresh clone
through the starved pool, so the gate fails correctly. The seed-
completeness check in conftest now covers the reserved repo too.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(git-leak): harden harness teardown and tighten assertions (PR review)

Follow-up to PR review of the git-leak/resilience test bed:

- hard_reset: always restart opal_server + wait_healthy via a finally, so a
  failed redis FLUSHALL can't leave the server stopped (which would fail every
  later session-scoped test and, running in a test finally, mask the result).
- delete_all_scopes drain: a transient /internal read error no longer counts as
  a successful drain (was `except: return`); keep polling to the deadline so a
  not-yet-drained cache can't leak into the next test once PR2 lands.
- use stats(samples=1) for the zero-waiting drain/empty polls (the peak-merge
  only matters for load assertions; this also drops 3x HTTP per poll).
- resilience: narrow broad `except Exception` to requests.RequestException
  (and RuntimeError for wait_healthy timeout) so harness bugs surface instead of
  masquerading as "never served"/"never recovered".
- resilience: collapse `assert opal.stats()` + redundant re-read into one read.
- debug_stats_test: assert rss_kb > 0 on Linux (was `>= 0`, which passed for the
  wrong reason where /proc is absent and RSS reads fall back to 0).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(git-leak): make PR4/PR5 tests genuine gates (address PR review)

Zeev's gate-coverage review found only 2 of 5 flagship tests (churn #1,
offline #4) were genuine fail-now / pass-after gates. Lift the other three:

- #5 broadcaster: run 2 workers (OPAL_TEST_WORKERS) so the Postgres backbone
  is actually fanned out cross-worker (references/debug-pubsub.md §3-4), and
  assert the gunicorn worker PIDs are unchanged across a transient bounce --
  the in-place-reconnect signal that distinguishes #915 (PER-15065) from a
  plain worker respawn. Prove recovery via a servable post-bounce scope, not
  /internal cache counts (per-process, non-deterministic on 2 workers).
- #3 boot: key completion on "all scopes served" (GET /scopes/{id}/policy ==
  200) instead of repo_locks (set at fetch start, so it undercounts the final
  clone); document the PR4 tight-BOOT_TARGET_SECONDS carry-forward.
- #2 repeat-sync: rename to test_repeat_sync_rss_stays_bounded and drop the
  tautological len(repos) assertion; RSS is the sole (load-bearing) gate.

Adds worker_pids() (/proc-based, matched host-side so the scan can't count its
own sh -c wrapper) and the opal_multiworker fixture (recreate to 2 workers,
restore to 1 on teardown).

Validated live (--boot-scopes=50): #2/#3 pass, #5 passes (worker PIDs held
across the bounce, post-bounce scope served), #1/#4 fail for the right reason
(PR2/PR3 not landed).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(git-leak): address PR review round 3 (fixture robustness + cleanup)

Zeev's latest inline batch:
- pytest.ini: self-root the suite (app-tests/git-leak/pytest.ini) so
  `cd app-tests/git-leak && pytest --boot-scopes=N` is deterministic across
  pytest versions/cwd. (The documented command already works -- testpaths only
  applies when pytest runs from the rootdir -- but this makes it explicit.)
- compose(): add a subprocess timeout (default 1200s) so a wedged up/wait/build
  fails fast instead of hanging session-scoped fixture setup to the CI job limit
  (pytest-timeout does not cover fixture setup).
- delete_all_scopes(): cut the drain wait 20s -> 3s; on master the caches can't
  purge (the leak this gates), so the old wait burned ~40s of dead time per test
  across setup+teardown.
- seed_gitea.py: inject push creds scheme-agnostically (urllib.parse) instead of
  string-replacing "http://"; drop the unused /seed-output token artifact and the
  seed-output volume (host uses basic auth, never the token).

The order-dependent bounce signal (test_resilience.py) was already fixed in
8e24cb0 (asserts GET /scopes/post-bounce/policy == 200, not a delta on a shared
process-global counter).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(git-leak): harden gates per multi-agent review (H1 + M1-M6)

Addresses the HIGH and all MEDIUM findings from the opal-development /
python-pro / backend-architect microreview:

- H1 (#5 vacuous pass): the broadcaster gate now positively verifies a
  disconnect+reconnect actually happened, not just that no respawn occurred.
  New helpers.broadcaster_connect_count() counts the reconnecting broadcaster's
  "listener connected to channel" log line; the test asserts it increased across
  the bounce (paired with worker PIDs unchanged = in-place, not respawn).
- M1 (#4): move the 40 executor-saturating PUTs inside the try/finally so a PUT
  failure still runs hard_reset() instead of leaking hung clone threads into the
  session stack.
- M2 (#1/#2): _wait_until now treats a transient requests error from opal.stats()
  as "not yet" and retries, instead of ERRORing the test.
- M3 (#3): measure a deterministic pure-cold boot (--force-recreate wipes the
  ephemeral FS -> preload cold-clones all N from Redis) instead of a
  nondeterministic warm/cold mix, so PR4's tight BOOT_TARGET_SECONDS can gate.
- M4: verify the single-worker invariant -- opal_multiworker teardown asserts the
  stack is back to 1 worker, and the opal fixture asserts single-worker at setup,
  so a botched restore fails loudly instead of silently breaking cache gates.
- M5 (#4): correct the reserved-probe comment (serving shares the fetch executor,
  so a shared repo would be starved on serve too; the probe additionally
  exercises the clone).
- M6: gitea-admin retries on "database is locked" (CLI mutating live SQLite);
  rewritten as a `|` literal block so the create call stays on one line.

Validated live (--boot-scopes=20): #2/#3 pass, #5 passes (reconnect count
increased across the bounce + PIDs unchanged + scope served), #1/#4 fail for the
right reason.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(git-leak): add PR2 over-purge/update-path gates + during-outage publish guard (review items 1-4)

Coverage additions from the test-coverage review pass:

1. Unit tests for the server.py wiring: build the real OpalServer app and
   assert /internal/git-fetcher-cache-stats is mounted iff
   DEBUG_INTERNAL_STATS is on — removing the register call from
   _init_fast_api_app now fails the unit suite.
2. test_shared_repo_survives_sibling_scope_delete: deleting one of two
   scopes sharing a repo URL must not purge the URL-keyed cache entry or
   break the survivor (guards PR2 against over-purging; churn only covers
   the all-scopes-gone direction).
3. test_scope_repoint_releases_old_repo_cache: PUT /scopes is
   create-or-update, so re-pointing a scope orphans the old URL's cache
   entries — a leak path churn (delete-only) never takes. Red until PR2's
   purge covers updates too.
4. Postgres-bounce test: bounce_postgres gained a during= callback (with a
   finally that restores Postgres even if it raises) and the test now PUTs
   a scope mid-outage, asserting (d) it becomes servable after recovery —
   its sync trigger must be buffered/replayed across the gap, not silently
   dropped. Uses a distinct seeded repo from (c) so a stale on-disk clone
   can't satisfy it vacuously.

Validated live (--boot-scopes=2): #2 and the bounce test pass; #3 fails
for the right reason (caches stuck at 2/2/2 after the delete — PR2 not
landed). Unit suite: 52 passed; pre-commit clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: zivxx <zivxx1@gmail.com>
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.

2 participants