Skip to content

Fix: increment postsCount by local writes in read-after-write profile munge#175

Open
peroumal1 wants to merge 1 commit intoblacksky-algorithms:mainfrom
peroumal1:fix/164-read-after-write-stale-posts-count
Open

Fix: increment postsCount by local writes in read-after-write profile munge#175
peroumal1 wants to merge 1 commit intoblacksky-algorithms:mainfrom
peroumal1:fix/164-read-after-write-stale-posts-count

Conversation

@peroumal1
Copy link
Copy Markdown

@peroumal1 peroumal1 commented Apr 9, 2026

Fixes #164

What

update_profile_detailed in read_after_write/viewer.rs now accepts a local_posts_count: usize parameter and adds it to the upstream posts_count via a new apply_local_posts_count helper. Both call sites (get_profile_munge and get_profiles_munge) pass local.posts.len().

Why

After createRecord, the read-after-write munge was overlaying profile-record fields (displayName, description, avatar, banner) but passing upstream followers_count, follows_count, and posts_count through unchanged. The stale postsCount caused bsky.app to render a blank profile shell immediately after a post, because the appview response was inconsistent with the local feed state.

Testing

3 unit tests for apply_local_posts_count in read_after_write::viewer::tests:

  • local posts are added to the upstream count
  • zero local posts leave the count unchanged
  • None upstream count stays None (unknown, not guessed)

Full LocalViewer-level integration testing is not feasible without a live database, which is noted as a known limitation.

Checklist

  • Changes have been tested, including unit tests
  • Implementation aligns with the canonical TypeScript implementation and/or the atproto spec
  • Relevant documentation — n/a
  • Code is formatted (cargo fmt)
  • Examples: unit tests serve as usage examples

@afbase
Copy link
Copy Markdown
Collaborator

afbase commented Apr 10, 2026

isn't this going to double count the posts?

@afbase
Copy link
Copy Markdown
Collaborator

afbase commented Apr 10, 2026

I think the count between the appview and what the PDS here is going to get confused

@peroumal1
Copy link
Copy Markdown
Author

peroumal1 commented Apr 11, 2026

Hey,
You might be right, I'm looking for now for a way to test this further, will update the PR accordingly if needed.

peroumal1 pushed a commit to peroumal1/rsky that referenced this pull request Apr 11, 2026
Add mock AppView test harness + read-after-write integration test
5eb7f5b 
- AppViewConfig + RocketConfig.app_view: inject a test AppView URL/DID
  into build_rocket() without touching env vars, enabling parallel tests
- dev_mode auto-enabled when AppView is injected (bypasses is_safe_url
  localhost check in pipethrough)
- MockAppView: minimal tokio HTTP/1.1 server bound to 127.0.0.1:0,
  returns fixed JSON with configurable atproto-repo-rev header
- get_client_with_appview() + create_session() test helpers
- get_profile_postscount_incremented_after_local_create_record:
  sentinel record → capture rev → write profile+post → assert
  postsCount = upstream(5) + local_posts(1) = 6 (tests PR blacksky-algorithms#175)
peroumal1 pushed a commit to peroumal1/rsky that referenced this pull request Apr 11, 2026
Fix: increment postsCount by local writes in read-after-write profile…
eea9cea 
… munge (issue blacksky-algorithms#164)

Pass local.posts.len() into update_profile_detailed and add it to the
upstream posts_count via apply_local_posts_count(). Previously the munge
only overlaid profile-record fields (displayName, description, avatar,
banner) but preserved stale upstream counters unchanged, causing bsky.app
to render a blank profile shell immediately after a post.

Add mock AppView test harness + read-after-write integration test

- AppViewConfig + RocketConfig.app_view: inject a test AppView URL/DID
  into build_rocket() without touching env vars, enabling parallel tests
- dev_mode auto-enabled when AppView is injected (bypasses is_safe_url
  localhost check in pipethrough)
- MockAppView: minimal tokio HTTP/1.1 server bound to 127.0.0.1:0,
  returns fixed JSON with configurable atproto-repo-rev header
- get_client_with_appview() + create_session() test helpers
- get_profile_postscount_incremented_after_local_create_record:
  sentinel record → capture rev → write profile+post → assert
  postsCount = upstream(5) + local_posts(1) = 6 (tests PR blacksky-algorithms#175)

Fix read-after-write test: activate account after creation

The create_account helper passes a hardcoded DID, which triggers the
"import from another PDS" code path in validate_inputs_for_local_pds
and sets deactivated = true. AccessStandardIncludeChecks (used by
createRecord) rejects deactivated accounts with HTTP 400, causing the
sentinel createRecord assertion to fail in CI.

Add create_active_account helper that clears deactivatedAt via a direct
SQL UPDATE after account creation. Existing tests are unaffected.

Fix Sequencer DB connection in integration tests

Sequencer::sequence_evt (and all other Sequencer DB methods) called
establish_connection_for_sequencer() which reads DATABASE_URL from the
environment. Integration tests inject the DB URL directly into Rocket's
Figment and never set DATABASE_URL, causing every createRecord (and any
other sequenced write) to fail with a connection error -> HTTP 500.

Add db_url: String to Sequencer, set via new()'s third parameter.
Falls back to DATABASE_URL env var when None is passed (preserves
production behavior). build_rocket passes the already-known db_url;
subscribe_repos passes None (env var fallback).

Fix test assertion: getProfile munge wraps profile in HandlerResponse envelope

The munged response serialises as {encoding, body: {...profile...}, headers}.
Assertion was checking body["postsCount"] instead of body["body"]["postsCount"].

Add read-after-write guard path tests + pre-pull Postgres in CI

Three new integration tests covering the paths that bypass the munge:

- get_profile_unauthenticated_returns_raw_appview_response
  requester=None → HandlerPipeThrough immediately; raw AppView JSON,
  postsCount at top level equals upstream value.

- get_profile_appview_current_skips_munge
  atproto-repo-rev = HEAD → local.count==0 → HandlerPipeThrough;
  confirms the fix does not spuriously increment when AppView is current.

- get_profile_no_local_profile_record_postscount_unchanged
  local posts exist after sentinel rev but no "self" profile record →
  munge enters but exits early (local.profile=None) → HandlerResponse
  envelope with upstream postsCount unchanged.

Also pre-pull postgres:11-alpine in CI before cargo test to eliminate
the per-test Docker image pull from testcontainer startup time.

Fix failing unauthenticated test: replace with no-rev-header path

The unauthenticated path was unreachable — AccessStandard always returns
Outcome::Error on missing auth, so requester = None is dead code.

Replace with get_profile_no_rev_header_returns_raw_appview_response which
exercises the real HandlerPipeThrough path: AppView response missing the
atproto-repo-rev header → rev = None → read_after_write_internal returns
HandlerPipeThrough immediately → raw JSON forwarded with no envelope.

Also refactor MockAppView: add start_without_rev() constructor and extract
shared start_inner() to avoid duplication.
peroumal1 pushed a commit to peroumal1/rsky that referenced this pull request Apr 11, 2026
Fix: increment postsCount by local writes in read-after-write profile…
ebdd31e 
… munge (issue blacksky-algorithms#164)

Pass local.posts.len() into update_profile_detailed and add it to the
upstream posts_count via apply_local_posts_count(). Previously the munge
only overlaid profile-record fields (displayName, description, avatar,
banner) but preserved stale upstream counters unchanged, causing bsky.app
to render a blank profile shell immediately after a post.

Add mock AppView test harness + read-after-write integration test

- AppViewConfig + RocketConfig.app_view: inject a test AppView URL/DID
  into build_rocket() without touching env vars, enabling parallel tests
- dev_mode auto-enabled when AppView is injected (bypasses is_safe_url
  localhost check in pipethrough)
- MockAppView: minimal tokio HTTP/1.1 server bound to 127.0.0.1:0,
  returns fixed JSON with configurable atproto-repo-rev header
- get_client_with_appview() + create_session() test helpers
- get_profile_postscount_incremented_after_local_create_record:
  sentinel record → capture rev → write profile+post → assert
  postsCount = upstream(5) + local_posts(1) = 6 (tests PR blacksky-algorithms#175)

Fix read-after-write test: activate account after creation

The create_account helper passes a hardcoded DID, which triggers the
"import from another PDS" code path in validate_inputs_for_local_pds
and sets deactivated = true. AccessStandardIncludeChecks (used by
createRecord) rejects deactivated accounts with HTTP 400, causing the
sentinel createRecord assertion to fail in CI.

Add create_active_account helper that clears deactivatedAt via a direct
SQL UPDATE after account creation. Existing tests are unaffected.

Fix Sequencer DB connection in integration tests

Sequencer::sequence_evt (and all other Sequencer DB methods) called
establish_connection_for_sequencer() which reads DATABASE_URL from the
environment. Integration tests inject the DB URL directly into Rocket's
Figment and never set DATABASE_URL, causing every createRecord (and any
other sequenced write) to fail with a connection error -> HTTP 500.

Add db_url: String to Sequencer, set via new()'s third parameter.
Falls back to DATABASE_URL env var when None is passed (preserves
production behavior). build_rocket passes the already-known db_url;
subscribe_repos passes None (env var fallback).

Fix test assertion: getProfile munge wraps profile in HandlerResponse envelope

The munged response serialises as {encoding, body: {...profile...}, headers}.
Assertion was checking body["postsCount"] instead of body["body"]["postsCount"].

Add read-after-write guard path tests + pre-pull Postgres in CI

Three new integration tests covering the paths that bypass the munge:

- get_profile_unauthenticated_returns_raw_appview_response
  requester=None → HandlerPipeThrough immediately; raw AppView JSON,
  postsCount at top level equals upstream value.

- get_profile_appview_current_skips_munge
  atproto-repo-rev = HEAD → local.count==0 → HandlerPipeThrough;
  confirms the fix does not spuriously increment when AppView is current.

- get_profile_no_local_profile_record_postscount_unchanged
  local posts exist after sentinel rev but no "self" profile record →
  munge enters but exits early (local.profile=None) → HandlerResponse
  envelope with upstream postsCount unchanged.

Also pre-pull postgres:11-alpine in CI before cargo test to eliminate
the per-test Docker image pull from testcontainer startup time.

Fix failing unauthenticated test: replace with no-rev-header path

The unauthenticated path was unreachable — AccessStandard always returns
Outcome::Error on missing auth, so requester = None is dead code.

Replace with get_profile_no_rev_header_returns_raw_appview_response which
exercises the real HandlerPipeThrough path: AppView response missing the
atproto-repo-rev header → rev = None → read_after_write_internal returns
HandlerPipeThrough immediately → raw JSON forwarded with no envelope.

Also refactor MockAppView: add start_without_rev() constructor and extract
shared start_inner() to avoid duplication.
peroumal1 pushed a commit to peroumal1/rsky that referenced this pull request Apr 12, 2026
Add mock AppView test harness + read-after-write integration test
098873a 
- AppViewConfig + RocketConfig.app_view: inject a test AppView URL/DID
  into build_rocket() without touching env vars, enabling parallel tests
- dev_mode auto-enabled when AppView is injected (bypasses is_safe_url
  localhost check in pipethrough)
- MockAppView: minimal tokio HTTP/1.1 server bound to 127.0.0.1:0,
  returns fixed JSON with configurable atproto-repo-rev header
- get_client_with_appview() + create_session() test helpers
- get_profile_postscount_incremented_after_local_create_record:
  sentinel record → capture rev → write profile+post → assert
  postsCount = upstream(5) + local_posts(1) = 6 (tests PR blacksky-algorithms#175)
peroumal1 pushed a commit to peroumal1/rsky that referenced this pull request Apr 12, 2026
Add mock AppView test harness + read-after-write integration test
9c1b767 
- AppViewConfig + RocketConfig.app_view: inject a test AppView URL/DID
  into build_rocket() without touching env vars, enabling parallel tests
- dev_mode auto-enabled when AppView is injected (bypasses is_safe_url
  localhost check in pipethrough)
- MockAppView: minimal tokio HTTP/1.1 server bound to 127.0.0.1:0,
  returns fixed JSON with configurable atproto-repo-rev header
- get_client_with_appview() + create_session() test helpers
- get_profile_postscount_incremented_after_local_create_record:
  sentinel record → capture rev → write profile+post → assert
  postsCount = upstream(5) + local_posts(1) = 6 (tests PR blacksky-algorithms#175)

Fix read-after-write test: activate account after creation

The create_account helper passes a hardcoded DID, which triggers the
"import from another PDS" code path in validate_inputs_for_local_pds
and sets deactivated = true. AccessStandardIncludeChecks (used by
createRecord) rejects deactivated accounts with HTTP 400, causing the
sentinel createRecord assertion to fail in CI.

Add create_active_account helper that clears deactivatedAt via a direct
SQL UPDATE after account creation. Existing tests are unaffected.

Fix Sequencer DB connection in integration tests

Sequencer::sequence_evt (and all other Sequencer DB methods) called
establish_connection_for_sequencer() which reads DATABASE_URL from the
environment. Integration tests inject the DB URL directly into Rocket's
Figment and never set DATABASE_URL, causing every createRecord (and any
other sequenced write) to fail with a connection error -> HTTP 500.

Add db_url: String to Sequencer, set via new()'s third parameter.
Falls back to DATABASE_URL env var when None is passed (preserves
production behavior). build_rocket passes the already-known db_url;
subscribe_repos passes None (env var fallback).

Fix test assertion: getProfile munge wraps profile in HandlerResponse envelope

The munged response serialises as {encoding, body: {...profile...}, headers}.
Assertion was checking body["postsCount"] instead of body["body"]["postsCount"].

Add read-after-write guard path tests + pre-pull Postgres in CI

Three new integration tests covering the paths that bypass the munge:

- get_profile_unauthenticated_returns_raw_appview_response
  requester=None → HandlerPipeThrough immediately; raw AppView JSON,
  postsCount at top level equals upstream value.

- get_profile_appview_current_skips_munge
  atproto-repo-rev = HEAD → local.count==0 → HandlerPipeThrough;
  confirms the fix does not spuriously increment when AppView is current.

- get_profile_no_local_profile_record_postscount_unchanged
  local posts exist after sentinel rev but no "self" profile record →
  munge enters but exits early (local.profile=None) → HandlerResponse
  envelope with upstream postsCount unchanged.

Also pre-pull postgres:11-alpine in CI before cargo test to eliminate
the per-test Docker image pull from testcontainer startup time.

Fix failing unauthenticated test: replace with no-rev-header path

The unauthenticated path was unreachable — AccessStandard always returns
Outcome::Error on missing auth, so requester = None is dead code.

Replace with get_profile_no_rev_header_returns_raw_appview_response which
exercises the real HandlerPipeThrough path: AppView response missing the
atproto-repo-rev header → rev = None → read_after_write_internal returns
HandlerPipeThrough immediately → raw JSON forwarded with no envelope.

Also refactor MockAppView: add start_without_rev() constructor and extract
shared start_inner() to avoid duplication.
Fix: increment postsCount by local writes in read-after-write profile…
58609a6 
… munge (issue blacksky-algorithms#164)

Pass local.posts.len() into update_profile_detailed and add it to the
upstream posts_count via apply_local_posts_count(). Previously the munge
only overlaid profile-record fields (displayName, description, avatar,
banner) but preserved stale upstream counters unchanged, causing bsky.app
to render a blank profile shell immediately after a post.
@peroumal1 peroumal1 force-pushed the fix/164-read-after-write-stale-posts-count branch from c636d55 to 58609a6 Compare April 12, 2026 07:50
peroumal1 pushed a commit to peroumal1/rsky that referenced this pull request Apr 12, 2026
Add mock AppView test harness + read-after-write integration test
9327201 
- AppViewConfig + RocketConfig.app_view: inject a test AppView URL/DID
  into build_rocket() without touching env vars, enabling parallel tests
- dev_mode auto-enabled when AppView is injected (bypasses is_safe_url
  localhost check in pipethrough)
- MockAppView: minimal tokio HTTP/1.1 server bound to 127.0.0.1:0,
  returns fixed JSON with configurable atproto-repo-rev header
- get_client_with_appview() + create_session() test helpers
- get_profile_postscount_incremented_after_local_create_record:
  sentinel record → capture rev → write profile+post → assert
  postsCount = upstream(5) + local_posts(1) = 6 (tests PR blacksky-algorithms#175)

Fix read-after-write test: activate account after creation

The create_account helper passes a hardcoded DID, which triggers the
"import from another PDS" code path in validate_inputs_for_local_pds
and sets deactivated = true. AccessStandardIncludeChecks (used by
createRecord) rejects deactivated accounts with HTTP 400, causing the
sentinel createRecord assertion to fail in CI.

Add create_active_account helper that clears deactivatedAt via a direct
SQL UPDATE after account creation. Existing tests are unaffected.

Fix Sequencer DB connection in integration tests

Sequencer::sequence_evt (and all other Sequencer DB methods) called
establish_connection_for_sequencer() which reads DATABASE_URL from the
environment. Integration tests inject the DB URL directly into Rocket's
Figment and never set DATABASE_URL, causing every createRecord (and any
other sequenced write) to fail with a connection error -> HTTP 500.

Add db_url: String to Sequencer, set via new()'s third parameter.
Falls back to DATABASE_URL env var when None is passed (preserves
production behavior). build_rocket passes the already-known db_url;
subscribe_repos passes None (env var fallback).

Fix test assertion: getProfile munge wraps profile in HandlerResponse envelope

The munged response serialises as {encoding, body: {...profile...}, headers}.
Assertion was checking body["postsCount"] instead of body["body"]["postsCount"].

Add read-after-write guard path tests + pre-pull Postgres in CI

Three new integration tests covering the paths that bypass the munge:

- get_profile_unauthenticated_returns_raw_appview_response
  requester=None → HandlerPipeThrough immediately; raw AppView JSON,
  postsCount at top level equals upstream value.

- get_profile_appview_current_skips_munge
  atproto-repo-rev = HEAD → local.count==0 → HandlerPipeThrough;
  confirms the fix does not spuriously increment when AppView is current.

- get_profile_no_local_profile_record_postscount_unchanged
  local posts exist after sentinel rev but no "self" profile record →
  munge enters but exits early (local.profile=None) → HandlerResponse
  envelope with upstream postsCount unchanged.

Also pre-pull postgres:11-alpine in CI before cargo test to eliminate
the per-test Docker image pull from testcontainer startup time.

Fix failing unauthenticated test: replace with no-rev-header path

The unauthenticated path was unreachable — AccessStandard always returns
Outcome::Error on missing auth, so requester = None is dead code.

Replace with get_profile_no_rev_header_returns_raw_appview_response which
exercises the real HandlerPipeThrough path: AppView response missing the
atproto-repo-rev header → rev = None → read_after_write_internal returns
HandlerPipeThrough immediately → raw JSON forwarded with no envelope.

Also refactor MockAppView: add start_without_rev() constructor and extract
shared start_inner() to avoid duplication.
@peroumal1
Copy link
Copy Markdown
Author

peroumal1 commented Apr 12, 2026

Ok so, back with more tests! I was able to confirm the implementation won't double count the posts.

To validate it I built a mock AppView test harness for rsky-pds. The test spins up a full Rocket + Postgres instance and replaces the AppView HTTP endpoint with a lightweight mock server that returns a controlled response (like our case here).

The integration test then: 1. Creates an account and writes a post locally via the PDS
2. Calls app.bsky.actor.getProfile while the AppView mock returns the stale count
3. Asserts that the munged response reflects postsCount + local_writes — confirming apply_local_posts_count fires correctly in the read-after-write path

The testing logic lives in a separate branch in my fork.. Happy to discuss the testing approach further in a separate discussion, and open a PR for this as well if this is something that would be welcome.

@afbase
Copy link
Copy Markdown
Collaborator

afbase commented Apr 16, 2026

I think the approach to consider is to follow the atproto reference pds read-after-write implementation that doesn't account for postCounts. An appview should eventually stabilize the post count.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

PDS: Read-after-write profile munging preserves stale upstream postsCount after local writes

2 participants

@peroumal1 @afbase