Skip to content

fix(hook-context-intelligence): forwarding-path reliability & diagnostics hardening - #95

Open
Diego Colombo (colombod) wants to merge 6 commits into
mainfrom
fix/forwarding-diagnostics-reliability
Open

fix(hook-context-intelligence): forwarding-path reliability & diagnostics hardening#95
Diego Colombo (colombod) wants to merge 6 commits into
mainfrom
fix/forwarding-diagnostics-reliability

Conversation

@colombod

Copy link
Copy Markdown
Collaborator

What this changes

Four independent reliability/diagnosability fixes to hook-context-intelligence's
server-forwarding path, plus a small test-import lint fix. Each is an atomic commit;
none changes the on-disk event format or the guarantee that events are written durably
to events.jsonl before any dispatch is attempted.

1. Retry HTTP 408 instead of dropping the event

_classify_http_outcome treated 408 (Request Timeout) as permanent, so a timed-out POST
was skipped rather than retried. 408 is transient by definition (RFC 7231 permits
repeating the request unchanged); it now retries with backoff like 429/5xx. Other codes
are unchanged.

2. Make auth-token failures diagnosable

When producing the Authorization header failed, the only durable record was a constant
string — every distinct cause (expired token, wrong audience, broker unavailable, a
masked bug) produced the same uninformative line, and a burst collapsed to one. The
durable forwarding record now carries the exception type + message and is written
per-failure (moved out of the console rate-limit branch); console logs stay rate-limited.

3. Positive liveness heartbeat

The diagnostics sink only ever wrote on a problem, so an empty log was ambiguous between
"healthy", "never started", and "no sessions ran". A destination now writes one
delivery_ok record on its first successful delivery per session. An empty log now means
something specific.

4. Fault-isolate per-destination dispatcher construction — completes #85

on_session_ready built all dispatchers in a bare comprehension. If constructing one
destination's dispatcher raised (e.g. the azure-identity credential chain failing to
initialise for an entra destination — an environmental failure that config validation
cannot screen), the exception propagated out of on_session_ready; the kernel swallows
it, so set_dispatchers() and event registration never ran. One misconfigured
destination silently disabled forwarding for every destination, including healthy ones,
with no diagnostic anywhere.

Each construction is now isolated in its own try/except: the failing destination is
skipped, the rest still forward, and the failure is recorded durably in the
forwarding-diagnostics JSONL (dispatcher_construction_failed, carrying the exception
type + message) as well as the kernel log.

This completes PR #85. #85 made validate_destinations() degrade per-destination at
the config-validation layer so a misconfigured destination can't take down local capture.
This applies the same per-destination isolation one layer deeper — at dispatcher
construction — which #85 left unguarded, and routes the diagnostic into the same
forwarding-diagnostics JSONL as the source of truth.

End-user impact

  • Transient server timeouts no longer silently drop events out of the live graph.
  • Auth problems are diagnosable from the durable forwarding log alone — no need to have
    had DEBUG enabled before the fault occurred.
  • An empty forwarding log is now unambiguous: a heartbeat proves the destination was live.
  • A single bad destination can no longer silently disable forwarding for every other
    destination — and when one is dropped, it says so, durably, where operators look.

Validation

  • Unit: full module suite green (636 passed); ruff, ruff format, and pyright all clean.
  • End-to-end in isolated Digital-Twin environments (isolated Neo4j + context-intelligence
    server), each fix proven against a pre-fix control:
    • 408: fixed retries to delivery ([408,408,408,202]); pre-fix control loses the event (permanent_reject, 3 events dropped).
    • auth: durable record carries ClientAuthenticationError: …; 5 distinct faults → 5 distinct records.
    • heartbeat: exactly one delivery_ok per session over 23 delivered events.
    • construction: fixed emits dispatcher_construction_failed for the bad destination and the healthy destination still forwards (Neo4j node count confirmed); pre-fix control loses ALL forwarding silently.

@colombod

Copy link
Copy Markdown
Collaborator Author

What this improves for end users

Context Intelligence forwards each session's events to a shared graph server so the team's graph stays current. These fixes make that forwarding more reliable and, when something does go wrong, actually diagnosable — without changing how events are captured locally (they remain durably written to events.jsonl before any network call).

After this PR:

  • Fewer stale-graph gaps from transient timeouts. A server that momentarily times out a request (HTTP 408) used to make the bundle drop that event from live delivery. It now retries it, so the event reaches the graph on a later attempt instead of only via a manual re-upload.
  • Auth problems you can actually diagnose. When the bundle can't mint an auth token, the durable forwarding log used to record the same opaque line for every possible cause. It now records the actual exception type and message — so an expired login, a wrong audience, or a broker outage are told apart from the log alone, even if DEBUG wasn't enabled before the fault.
  • An empty log that means something. A healthy destination used to write nothing, so "no records" was ambiguous between "working fine", "never ran", and "silently broken". A destination now writes one delivery_ok heartbeat per session, so silence is unambiguous.
  • One bad destination can't silently take down the rest. If a single destination's credentials failed to initialise, the bundle previously aborted setup for every destination (including healthy ones) and left no diagnostic where anyone would look. Now the bad destination is skipped, every other destination keeps forwarding, and the failure is recorded durably in the forwarding log (dispatcher_construction_failed). This completes fix: misconfigured remote destination must not disable local capture #85, which handled the same "one bad destination shouldn't break the others" principle at the config-validation layer — this PR extends it to dispatcher construction.

Net effect: the team graph stays fresher, and when forwarding does degrade, the durable forwarding log now tells you what failed and which destination — instead of going quiet.

@colombod
Diego Colombo (colombod) force-pushed the fix/forwarding-diagnostics-reliability branch from 80008f5 to 67e60e2 Compare August 18, 2026 09:06
…import

test_mount_dispatcher.py imported amplifier_core.events.ALL_EVENTS without the
`# type: ignore[import-not-found]` suppression the module's own source
(__init__.py) already uses. ALL_EVENTS exists at runtime (real attribute);
pyright just cannot resolve amplifier_core.events statically. Matches the
existing in-repo convention. No behavior change.

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…nstruction

on_session_ready built dispatchers via a bare list comprehension over active
destinations. If _DestinationDispatcher.__init__ raised while constructing any
one destination -- build_auth_strategy() failing for an environmental reason
validate_destinations() cannot screen (e.g. the azure-identity credential chain
failing to initialise for an entra destination) -- the whole comprehension
aborted: set_dispatchers() was never reached and, because the kernel swallows
on_session_ready exceptions, event registration never ran either. One
misconfigured destination silently disabled forwarding for EVERY destination,
including the healthy ones, with no diagnostic anywhere.

Wrap each construction in its own try/except: skip only the failing destination,
keep the rest, and always fall through to set_dispatchers() + event registration.
Logged at module level (log.error, exc_info=True) because _record_forwarding_issue
is an instance method on the dispatcher that failed to construct.

This completes PR #85. #85 made validate_destinations() degrade per-destination at
the config-validation layer so a misconfigured destination cannot take down local
capture; this applies the same per-destination isolation one layer deeper -- at
dispatcher construction -- which #85 left unguarded.

Adds TestDispatcherConstructionFailure: on_session_ready does not raise, local
capture survives, and a valid sibling destination still gets its dispatcher when
another destination's auth construction fails.

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
_classify_http_outcome treated only 401/429/>=500 as transient; 408 (Request
Timeout) fell through to the _PERMANENT catch-all, so a timed-out request was
skipped instead of retried. 408 is transient by definition -- the server gave up
waiting for the request to complete; nothing about the payload, auth, or target
was rejected, and RFC 7231 explicitly permits repeating the request unchanged. It
belongs in the same retry-with-backoff bucket as 429 and 5xx.

Add 408 to the transient set and make the enumerating comments truthful. Other
status codes are unchanged: 401/429/5xx stay transient; 403/400/413/422/404/410
stay permanent. No overlap with the separate server-maintenance / Retry-After
pacing work.

Tests: 408 added to both TestTransientHttp parametrize lists plus an explicit
test_408_is_transient_not_permanent regression guard.

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…in durable record

When _strategy.headers() raised while producing the Authorization header, the only
durable artifact was the byte-identical constant "auth token production failed",
written from INSIDE the console rate-limit branch. The exception type reached only
the (non-durable, rate-limited) WARNING and the full traceback only DEBUG
(suppressed by default). Every distinct auth fault -- expired token, wrong
audience, broker unavailable, a masked TypeError -- produced one indistinguishable
durable record, and a burst within the rate-limit window collapsed to a single line.

Move the durable _record_forwarding_issue write OUTSIDE the rate-limit branch so
every failure is recorded, and carry type(exc).__name__ + str(exc) into the
record's detail. The console WARNING/DEBUG stay rate-limited to avoid log spam.
Write volume is bounded by dispatch backoff. Record schema is unchanged (only
detail content); the separate HTTP-401 auth_failure path is untouched.

Tests: TestAuthTokenUnavailableDurableRecord -- the durable record carries the
exception type + message (and http_status is None, distinguishing it from a real
401), and two distinct faults in quick succession each get their own record rather
than collapsing.

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…rtbeat

The forwarding-diagnostics sink only ever wrote on a PROBLEM (breaker_open,
permanent_reject, auth_token_unavailable, auth_failure, endpoint_not_found,
breaker_close, shutdown_undelivered, sustained_delivery_failure). A destination
that delivered successfully wrote nothing, ever, so an empty log was ambiguous
between "healthy and delivering", "never started", and "no sessions ran".

Add a one-time-per-session delivery_ok record, written the first time a
destination actually delivers (via a new _heartbeat_emitted flag +
_emit_delivery_heartbeat helper, wired into both the normal DELIVERED branch and
the breaker half-open probe-success branch). Fires at most once per dispatcher
lifetime so the happy path is not flooded; best-effort, never raises. An empty log
now unambiguously means "no delivery happened", not "possibly a silent outage".

Tests: TestDeliveryHeartbeat (first delivery writes exactly one delivery_ok; five
deliveries still produce exactly one; a never-delivering destination writes none).
Also updates test_close_clean_shutdown_writes_no_durable_record to assert its true
intent precisely now that the sole happy-path record is the delivery_ok heartbeat.

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
… durable in forwarding diagnostics

The per-destination construction guard reported failures only via module-level
log.error(exc_info=True) -- the kernel log, not the context-intelligence
forwarding-diagnostics JSONL where an operator investigating a forwarding problem
looks. That was inconsistent with the rest of this change, which makes the
forwarding JSONL the durable source of truth.

Also write a durable dispatcher_construction_failed record via the module-level
_write_forwarding_record (best-effort, never raises; does not need the failed
dispatcher instance), carrying type(exc).__name__ + str(exc) in detail and a null
http_status. The log.error(exc_info=True) is kept for the full traceback.

Test: test_construction_failure_writes_durable_forwarding_record asserts the
dropped destination leaves a dispatcher_construction_failed record whose detail
carries the exception type + message.

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@colombod
Diego Colombo (colombod) force-pushed the fix/forwarding-diagnostics-reliability branch from 67e60e2 to 23fea72 Compare August 20, 2026 09:19
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.

1 participant